Rigor Protocol contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

In Community contract, function escrow() is used to reduce debt when lender comed in terms with the builder and agent to reduce debt. It checks that all lender, builder and agent are signed the data.

But the issue is there is no nonce value in _data which means that anyone can use the same _data and _signature to call escrow() multiple times.

(
    uint256 _communityID,
    address _builder,
    address _lender,
    address _agent,
    address _project,
    uint256 _repayAmount,
    bytes memory _details
) = abi.decode(
        _data,
        (uint256, address, address, address, address, uint256, bytes)
    );

The result is attacker (may be builder) can call escrow() multiple times to reduce all the debt and cause loss of funds for lender (which is community owner).

Proof of Concept

Scenario

1. Alice is builder and Bob is community owner. Bob lent to Alice’s project 10000 DAI
2. Alice and Bob with a third party come up with a term (offchain) to reduce 1000 DAI in Alice’s debt. All Alice and Bob and the third party sign this term and someone call escrow() onchain. Now Alice debt is reduce to 9000 DAI
3. But Alice noticed the bug and use a clone wallet to call escrow() 9 more times to reduce all her debt (each time reduce 1000 DAI). This is possible because there is no mechanism like nonce value so escrow() can be call multiple times with the same params.

Tool Used

Manual Review

Recommended Mitigation Steps

Add a nonce value in _data param of escrow() function and increase it whenever someone call escrow()

Lines of code

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

Vulnerability details

Impact

In Project contract, the lendToProject() function might not be available to be called if there are a lot of Task in tasks[] list of project. It means that the project cannot be funded by either builder or community owner.

This can happen because lendToProject() used projectCost() function. And the loop in projectCost() did not have a mechanism to stop, it’s only based on the length taskCount, and may take all the gas limit. If the gas limit is reached, this transaction will fail or revert.

Same issue with toggleLendingNeeded() function which also call projectCost() function.

Proof of Concept

Function projectCost() did not have a mechanism to stop, only based on the taskCount.

function projectCost() public view override returns (uint256 _cost) {
    // Local instance of taskCount. To save gas.
    uint256 _length = taskCount;

    // Iterate over all tasks to sum their cost
    for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {
        _cost += tasks[_taskID].cost;
    }
}

There is no limit for builder when add task.

And function lendToProject() used projectCost() to check the new total lent value

require(
    projectCost() >= uint256(_newTotalLent),
    "Project::value>required"
);

Tools Used

Manual Review

Recommended Mitigation Steps

Consider keep value of projectCost() in a storage variable and update it when a task is added or updated accordingly.

Lines of code

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

Vulnerability details

Impact

In updateProjectHash() function, the _data encoded only _hash and _nonce value but not the _projectAddress. In case builder had 2 or more projects, the signature that builder used in updateProjectHash() can also be used in other projects by attackers.

(bytes memory _hash, uint256 _nonce) = abi.decode(
    _data,
    (bytes, uint256)
);

The result is wrong event emitted, which can make others application that build on top of this hash value malfunctioning.

And since there is a nonce, attacker can even front-running when builder try to updateProjectHash() of other projects. Builder will have to sign a new signature with bigger nonce, and attacker again can use this new signature to front-running the other projects.

Same issue with updateTaskHash() function when there is no _projectAddress value encoded in _data param.

Proof of Concept

Scenario

1. Alice is a builder and have 3 project A, B and C
2. Alice updated hash of project A 10 times. So attacker had signature for nonce value from 0 to 9 from project A
3. Now Alice is trying to update hash of project B and C. But each time when she sign the message with nonce from 0 to 9, attacker will front-run Alice and call updateProjectHash() with wrong hash value first (this hash is hash value of project A).
4. After 10 times failed to update hash value of project B, Alice now is able to call updateProjectHash() since attacker did not have signature with nonce = 10.
5. Attacker now again have signature for nonce = 10 and can use it to front-run other projects of Alice (A and C)

Tools Used

Manual Review

Recommended Mitigation Steps

Consider add _projectAddress encoded in _data param to prevent signature replay.

Summary

1. Not support fee-on-transfer tokens

Function Community.lendToProject function use a _amountToProject parameter but this parameter is not the actual transferred amount for fee-on-transfer / deflationary (or other rebasing) tokens.

Although sponsor said that they only support 3 tokens at this time, but they might consider this issue in the future before they want to add another token.

Impact

The actual lent amount might be lower than _amountToProject. The result is project is not receive enough funds to pay subcontractor and it broke the accouting mechanism.

Affected Codes

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

2. No limit in lenderFee set by HomeFi admin

There is no limit here

function replaceLenderFee(uint256 _newLenderFee)
    external
    override
    onlyAdmin
{
    // Revert if no change in lender fee
    require(lenderFee != _newLenderFee, "HomeFi::!Change");

    // Reset variables
    lenderFee = _newLenderFee;

    emit LenderFeeReplaced(_newLenderFee);
}

Affected Codes

• https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L185

3. A year has 365.25 days in average, not 365 days

In Community, when function returnToLender() calculates interest to return to lender, it assumed that a year has 365 days but in fact, it has 365.25 days in average.

Affected Codes

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

Summary

1. Caching returned values from function calls can save gas

External call to other contract is gas consuming and should avoid as much as possible.

For example, in Community.lendToProject() function, lenderFee() of _projectInstance is called 2 times. We should call only 1 time and cache the returned values to save gas.

Affected Codes

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

2. Cache length in the for loop and uncheck index

At each iteration of the loop, length is read from storage. We can cache the length and save gas per iteration.

Solidity 0.8.0 check safe math in every operation. Use uncheck to increase index can save gas.

For example

uint256 length = arr.length;
for (uint i; i < length; ) {
    // do stuff
    unchecked { ++i; }
}

Occurences

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