Rigor Protocol contest - Bahurum's results

Lines of code

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

Vulnerability details

Impact

A builder can, after it convened with the lender and an external agent to reduce his debt through escrow(), reuse the signature and pass it again to escrow() many times. This allows him to reduce his debt more than expected, leaving the lender at a loss.

Proof of Concept

Function Community.escrow() lacks signature replay protection. Once it has been called once successfully, the builder of a project can call it many times with the same _data and _signature and each time his debt will be reduced by _repayAmount.

Recommended Mitigation Steps

It is suggested to include a nonce into the signed data and checking it with the current nonce (as done elsewhere in the code) to avoid signature replays.

Lines of code

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

Vulnerability details

Impact

If there has been more than a change in a task's cost through mulitple calls to changeOrder(), signatures previously passed can be replayed by one party to change the price payed for the task without consent of the other parties by frontrunning call to setComplete().

Proof of Concept

A task's cost can be changed multiple times during negotiations between builder/contractor and subcontractor by calling changeOrder(). Let's say it is changed two times before setComplete() is finally called and the subcontractor is payed. If for example the second change sets a cost which is smaller than the first change, then the subcontractor can replay the first change by frontrunning setComplete(), changing the order cost just before payment and managing to be payed more than expected.

Recommended Mitigation Steps

It is suggested to include a nonce into the signed data and comparing it with the current nonce (as done elsewhere in the code) to avoid signature replays.

1. Confusing modifier name noChange():

In HomeFi.sol#L82 noChange modifier name is not coherent with previous modifier names, that say what is allowed (onlyAdmin and nonZero). According to the same logic, one should use mustChange.

2. Use of available modifier noChange():

In HomeFi.sol#L191 replace the check by the corresponding modifier noChange().

3. Unused/redundant code:

• In HomeFi.sol#L296 internal function mintNFT() with unused return.

• internal function _msgData() is never used (HomeFi.sol#L314 , Community.sol#L910)

• redundant casting of uint256 to uint256 at Project.sol#L200

4. Confusing natspec comment:

HomeFiProxy.sol#L32: comment

/// @dev mapping that maps contract initials with there implementation address

is confusing since the mapping points to the proxies and not the implementation addresses.

Same imprecision exists in natspec comment at HomeFiProxy.sol#L172.

5. One step ownership change

In HomeFiProxy.sol#L150 owner of proxyAdmin is changed through a single step procedure. This is a critical system parameter and it is recommended to implement a two-step change.