changeOrder can change completed tasks to inactive, which can be used to steal project money

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L160-L164

Vulnerability details

Impact

changeOrder calls unApprove on a task, but unApprove doesn't check for current task status (although comments suggests it should have onlyActive modifier). This allows to change completed tasks to inactive. Coupled with replay attack, this allows for malicious subcontractor to steal all project money in certain circumstances. Example flow:

1. Project has some tasks added and SC1 is assigned to all tasks
2. Contractor decides to re-assign task 1 to SC2 (calls changeOrder on task 1 to SC2)
3. Contractor decides to re-assign task 1 to SC3 (calls changeOrder on task 1 to SC3)
4. SC3 finishes task 1 and builder+contractor call setComplete on task 1, which pays the task cost to SC3
5. Malicious SC3 re-plays changeOrder to SC2 (using the same data and available signatures of builder and/or contractor), which puts task 1 into inactive state (changing to another subcontractor is required as the code only calls unApprove if subcontractor changes)
6. Immediately after that (in the same transaction), SC3 re-plays changeOrder to SC3 transaction (the same data + signatures)
7. Immediately after that, SC3 accepts invitation (task status is now active, and all alerts are set, including task funded alert)
8. Immediately after that, SC3 re-plays setComplete transaction (the same data + signatures), which completes task 1 again and pays out SC3 again
9. SC3 repeats steps 5-8 to get all available (allocated but not paid) money from the project.

There are also more complex scenarios possible, where changeOrder changes task cost only, as unApprove is called in such case too (together with unAllocateFunds) but only if there is not enough allocated funds to pay increased task cost, so it's harder to use.

Proof of Concept

Copy these to test/Hack.ts and test/utils/hack.ts run:
yarn test test/Hack.ts

https://gist.github.com/panprog/e9d5ad24b9bdf4ea6efabad5da385d0b

Recommended Mitigation Steps

Do not allow unApprove to change task state from Complete (depending on intentended functionality it might be possible to call unApprove on Inactive tasks, or only for Active tasks).

[horsefacts]

Duplicate of #230