Project.changeOrder can be called multiple times with same signature

[code423n4]

Lines of code

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

Vulnerability details

Impact

There are no nonces as part of the data for updating tasks in function changeOrder. It is possible that a builder and sub-contractor might agree to a price increase, followed by another price increase later by signing appropriate messages. However, the builder can replay the original message and signature reducing the price down to the first price which is lower than the final price.

Proof of Concept

• Assume builder has no contractor but does have sub-contractors
• Builder and sub-contractor signed message to change task 1 cost from $500 to $600
• Later, when they realise things are going to cost more, the builder and sub-contractor sign a new message to change task 1 cost from $600 to $1000.
• Builder replays the first signed message to reduce the cost of task 1 back down to $600
• when setComplete is eventually called the sub-contractor only gets $600 even though a new agreement existed for $1000.

Recommended Mitigation Steps

Add a nonce field to the Task struct and make the nonce part of the changeOrder function. See below.

diff --git a/contracts/Project.sol b/contracts/Project.sol
index 4a1dcf5..ddb480c 100644
--- a/contracts/Project.sol
+++ b/contracts/Project.sol
@@ -393,8 +393,9 @@ contract Project is
             uint256 _taskID,
             address _newSC,
             uint256 _newCost,
+            uint256 _nonce,
             address _project
-        ) = abi.decode(_data, (uint256, address, uint256, address));
+        ) = abi.decode(_data, (uint256, address, uint256, uint256, address));

         // If the sender is disputes contract, then do not check for signatures.
         if (_msgSender() != disputes) {
@@ -404,6 +405,8 @@ contract Project is

         // Revert if decoded project address does not match this contract. Indicating incorrect _data.
         require(_project == address(this), "Project::!projectAddress");
+        require(_nonce == tasks[_taskID].nonce,"Project::!nonce");
+        tasks[_taskID].nonce = tasks[_taskID].nonce + 1;

diff --git a/contracts/libraries/Tasks.sol b/contracts/libraries/Tasks.sol
index a7b4815..d186beb 100644
--- a/contracts/libraries/Tasks.sol
+++ b/contracts/libraries/Tasks.sol
@@ -13,6 +13,7 @@ struct Task {
     address subcontractor; // Subcontractor of task
     // Lifecycle //
     TaskStatus state; // Status of task
+    uint256 nonce; // Nonce used for changing orders
     mapping(uint256 => bool) alerts; // Alerts of task
 }

[parv3213]

Duplicate of #339. But good recommendation.