Rigor Protocol contest - cryptphi's results

Lines of code

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

Vulnerability details

Impact

When Project.setComplete() is called , the task status is updated to Complete and task cost transferred to subcontractor . However it is possible for anyone to set the Task status for the same task back to Active then call Project.setComplete() again and repeat until remaining funds in project are drained.

Above is possible when the signature is forged or a signature replay after changing to a new subcontractor by calling changeOrder() then accepting the invitation by calling acceptInviteSC() . The former action will set TaskStatus to Inactive , and the latter would set TaskStatus to Active allowing the new subcontractor to call setComplete() and receive funds.

The process can be repeated until the funds lent to the project are drained.

Additionally, it is possible for the contractor to steal funds from the project after a setComplete() by approving a second address in approveHash(keccak256(data) (data param to be used in changeOrder call). Contract then makes the changeOrder() call after approving the hash, and the check would pass when it jumps to checkSignatureTask() due to the OR operation.

Proof of Concept

1. Bob is the current subcontractor for task 2 of Project ACME
2. Assume the cost for task 2 is 10000 USDC, Bob calls setComplete() and receives 10000 USDC and Task state is changed to Complete.
3. Alice while monitoring the network observes the setComplete call and copies the input used in the call.
4. Alice crafts the data and signature params (with her address as the new subcontractor) , but first hashes the data param and calls approveHash() with the hashed data as input.
5. Alice's data input has been added to the approvedHashes mapping.
6. Now Alice then calls changeOrder() with the crafted params in no.4 and she is able to pass the signature checks.
7. The new task Status has been changed to InActive.
8. Alice calls acceptInviteSC() to accept herself as the new subcontractor and set the Task State to Active
9. Alice then calls setComplete() and receives 10000 USDC.
10. Steps 4 - 9 are repeated switching between 2 accounts and funds received each time.

Tools Used

Manual review

Recommended Mitigation Steps

The Task state should not be changed once it is Completed.

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L887 https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L108-L115

Vulnerability details

Impact

It is possible to pass Signature Validity check with an SignatureDecoder.recoverKey() returns 0 whenever the builder and /or contractor have an existing approved hash for a data.

With occurrence of above, any user can call changeOrder or setComplete functions successfully after user approves data hashes.

Tools Used

Manual review

Recommended Mitigation Steps

There should be a require check for _recoveredSignature != 0 in checkSignatureValidity()

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Disputes.sol#L74-L81

Vulnerability details

Impact

Signature checks in project.sol can be bypassed for AddTask(), orderChange() and setComplete() functions when the caller of the functions is the Disputes.sol contract

The Disputes.initialize() function can be frontrunned by any user can and set their arbitrary homeFi contract.This would allow the arbitrary contract admin to resolve disputes and bypass signature checks in the 3 functions above in Project.sol contract.

One major impact is that a subcontractor can keep receiving funds for the same task multiple times by going through the process above bypassing the signature check section in Project.setComplete()

if (_msgSender() != disputes) {
            // Check signatures.
            checkSignatureTask(_data, _signature, _taskID);
        }

Proof of Concept

1. Bob is a subcontractor and raises a Task Pay action dispute, then proceeds to call Project.setComplete()
2. Bob receives funds, then sets up an arbitrary homeFi contract using the IHomeFi interface structure.
3. Bob deploys and initializes the arbitrary homeFi contract and is the admin
4. Bob then proceeds to Disputes.sol contract and calls Disputes.initialize() with the fake homeFi contract address to set the homeFi state to the fake homeFi contract and is able to call onlyAdmin functions.
5. Bob then calls Disputes.resolveDispute() with the necessary dispute information for _disputeID and _ratify as true
6. The dispute is resolved and executeTaskPay() is called which calls Project.setComplete(), bypassing the signature checks
7. Bob receives funds from the project contract for the same task again.

Tools Used

Manual review

Recommended Mitigation Steps

There should be an access control on the initialize() function to ensure it is called only by contract owner or contract deployer.

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L330 https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L386 https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L219

Vulnerability details

Impact

There are 3 instances where a check for ongoing dispute should have been done at the start of the functions, however there were no such checks.

Using setComplete as an example, it was documented in contest page .... If there is no ongoing dispute about that project, task status is updated and payment is made.... , looking at the code, when a TaskPay actionType dispute has been raised for a Task on the project, setComplete() can still be called successfully.

Above is similar to the orderChange() and addTask() functions.

Tools Used

Manual review

Recommended Mitigation Steps

A check for dispute present should be done at the start of the function calls for addTask, orderChange and setComplete functions.