If past subcontractor has been kicked, he can steal funds from new subcontractor for task completed.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L386-L490
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L330-L359

Vulnerability details

Impact

I have already commited the issue with the repeated calls of Project.changeOrder() by anyone.
Here is a worse usecase - past subcontractor can steal funds from new subcontractor for task completed.

Subcontractor's fund for completion can be stolen by a previous subcontractor (if any). Bad subcontractor can frontrun setComplete() transaction, if he calls Project.changeOrder() with already disclosed data+signature, set himself an active subcontractor and receives funds from task completed.

Proof of Concept

Project.changeOrder() is used to change subcontractors for tasks and costs for tasks.
Once this transaction is in history - it can be repeated by anyone using the same data+signature.
And here is setComplete() function.
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L330-L359
It makes a task completed and transfers money to a subcontractor.
The attack is:

1. setComplete() is called to pay to new subcontractor for his completed task, transaction is in mempool
2. past subcontractor (previously kicked) picks some changeOrder() in history, where he was a subcontractor
3. past subcontractor frontruns Agreements & Disclosures #1, he transact with changeOrder()
4. past subcontractor is now an active subcontractor
5. setComplete() happens, bad-subcontractor-fronrunner receives funds

Tools Used

Hardhat

Recommended Mitigation Steps

Add nonce in data or store used data+signature

[0xA5DF]

Similar to #95, but not a dupe (only reusing signature, not re-completing a task).
Sidenote: this will only work if you have the signature of current SC, i.e. current SC was the SC when the changeOrder() was called in the first place.

Disclosure: #95 is my bug

[parv3213]

I say it a duplicate of #95

[parv3213]

Duplicate of #339