Rigor Protocol contest - scaraven's results

Lines of code

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

Vulnerability details

Impact

Due to arithmetic rounding in returnToLender(), a builder can halve the APR paid to a community owner by paying every 1.9999 days. This allows a builder to drastically decrease the amount of interest paid to a community owner, which in turn allows them to advertise very high APR rates to secure funding, most of which they will not pay.

This issue occurs in the calculation of noOfDays in returnToLender() which calculates the number of days since interest has last been calculated. If a builder repays a very small amount of tokens every 1.9999 days, then the noOfDays will be rounded down to 1 days however lastTimestamp is updated to the current timestamp anyway, so the builder essentially accumulates only 1 day of interest after 2 days.

I believe this is high severity because a community owner can have a drastic decrease in interest gained from a loan which counts as lost rewards. Additionally, this problem does not require a malicious builder because if a builder pays at a wrong time, the loaner receives less interest anyway.

Proof of Concept

1. A community owner provides a loan of 500_000 tokens to a builder with an APR of 10% (ignoring treasury fees)
2. Therefore, the community owner will expect an interest of 136.9 tokens per day (273.9 per 2 days)
3. A builder repays 0.000001 tokens at lastTimestamp + 2*86400 - 1
4. noOfDays rounds down to 1 thereby accumulating 500_000 * 100 * 1 / 365000 = 136 tokens for 2 days
5. Therefore, the community owner only receives 5% APR with negligible expenses for the builder

Tools Used

VS Code

Recommended Mitigation Steps

There are two possible mitigations:

1. Add a scalar to noOfDays so that any rounding which occurs is negligible i.e.

        uint256 _noOfDays = (block.timestamp -
            _communityProject.lastTimestamp) * SCALAR / 86400; // 24*60*60


        /// Interest formula = (principal * APR * days) / (365 * 1000)
        // prettier-ignore
        uint256 _unclaimedInterest = 
                _lentAmount *
                _communities[_communityID].projectDetails[_project].apr *
                _noOfDays /
                365000 /
                SCALAR;

2. Remove the noOfDays calculation and calculate interest in one equation which reduces arithmetic rounding

uint256 _unclaimedInterest = 
                _lentAmount *
                _communities[_communityID].projectDetails[_project].apr *
                (block.timestamp -
            _communityProject.lastTimestamp) /
                365000 /
                86400;

Lines of code

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

Vulnerability details

Impact

Due to no check that a task has been completed when calling changeOrder(), If a subcontractor and builder and/or contractor team up, they can siphon project funds from other subcontractors by repeatedly completing the same task over and over again. By changing the subcontractor of a completed task through changeOrder() and reinviting the same subcontractor, the subcontractor can repeatedly setComplete() the same task.

As the funds being transferred have been allocated to other tasks, when other subcontractors try to complete their task, the transactions will revert due to insufficient balance in the contract. This essentially means that subcontractors will complete tasks for free and never be able to receive their rewards for those tasks, even if they raise a dispute, unless a new loan is created at which point the newest subcontractor will face the same issue.

Proof of Concept

1. Bob the builder (pun intended) has a project which has two tasks, each with a cost 50 USDC assigned to the corresponding subcontractors: Alice and Charlie
2. Alice, with the permission of Bob, calls setComplete() for her task and receives 50 USDC
3. Bob, with the permission of Alice, then calls changeOrder() on her task to change the subcontractor to Alice 2.0 (another address under Alice's control)
4. In changeOrder(), when the subcontractor is changed, the task status becomes INACTIVE however it still has the lifecycle TaskAllocated set to true
5. Alice 2.0 can now accept the invitation, setting the task state to ACTIVE and call setComplete() again
6. Alice 2.0 receives 50 USDC for her task, leaving 0 USDC in the contract
7. Charlie completes his task and the contract tries to send 50 USDC which fails

This attack does not even require two actors to work together, instead a builder (or contractor if they have been delegated to) can perform this attack independently (using a subcontractor owned by them). As this is a very easy way for a builder to steal funds from subcontractors without them realising (so it is most likely they complete the designated task) and there is no possible way for a subcontractor to receive their funds, even through a dispute, I believe high severity is suitable

Tools Used

VS Code

Recommended Mitigation Steps

Consider adding a check in changeOrder() to make sure that the task being modified has not been completed, e.g.

require(uint256(tasks[_taskID].state) != 3, "Task has been completed");

Otherwise, if the devs want to keep the functionality of being able to modify completed tasks, a convenient fix would be to make sure that if a completed task is modified, regardless whether it is cost or subcontractor, then funds must be unallocated and reallocated by pushing onto _changeOrderedTask()

Lines of code

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

Vulnerability details

Impact

When calculating projectCost(), the function loops through all tasks which have been added. The number of tasks is controlled by the builder and if the builder adds too many tasks then the function will fail due to block gas limit. This, in turn, prevents the project from receiving funding as the two functions which do so call projectCost(): lendToProject() in Project.sol and Community.sol and toggleLendingNeeded() in Community.sol.

As it is impossible for a builder to reduce the number of tasks, this would prevent the entire project from receiving funding which can be problematic if the project has already been half-funded or half-completed.

As this issue requires external circumstances (a builder adds numerous tasks) it causes one of the main features of the protocol to stop working, so I believe medium severity is suitable

Proof of Concept

1. A builder creates a project with a few tasks and calls toggleLendingNeeded()
2. In the meantime, the builder creates a very large number of tasks
3. Charlie, the community owner, tries to lend to the project with lendToProject() however the transaction reverts and fails

Tools Used

VS Code

Recommended Mitigation Steps

I think the simplest solution without adding new issues is to create a variable projectCost which updates every time a task is added/modified. Fetching the project cost would simply be returning the variable value.

Another solution would be to add a hard limit on the number of tasks which can be added, however I think the solution above is more elegant

Summary

Overall, the protocol was very well-developed with good readability. I very much liked how well the project was commented and how much work was put into helping wardens understand the protocol (particularly the flow descriptions and diagrams). As this project is quite special in that it has a defined use case outside the blockchain, it loses a lot of the security which comes from the blockchain, mainly trustlessness. There is a very heavy human element required for the protocol to work correctly, for example deciding disputes, which if compromised can cause massive damages and reputational loss.

Issue 1 Builder is not forced to pay back a loan

I have talked to the sponsors about this and agreed that this issue is mostly invalid because it is very likely that the builder must sign a legal contract. Additionally, they said that they plan to use KYC checks, so legal consequences can be enforced if a builder refuses to pay back a loan.

An idea I think which might be interesting is to use a smart contract which acts as an escrow contract. Instead of sending funds to the builder or subcontractor e.g. in autoWithdraw(), it may be sensible to send all funds to the escrow contract and only when the community owner approves can the funds be sent to a builder/subcontractor. This would prevent the builder running off with the loan because they would never own any of the assets at anytime. Additionally, this would prevent a builder from illegitimately sending funds to themselves indirectly (by completing their own tasks as a subcontractor) because all funding will have to be approved by the community owner. Of course, if the community owner does not want this responsibility, they can delegate all of this to the builder and so nothing changes.

I simply included this issue because I thought it might be an interesting idea which the sponsor may want to think about/build upon for a potential fix for this problem

Issue 2 HomeFi admin can easily rug all assets

One problem is that the HomeFi admin is able to control numerous operations in the protocol such as upgrading contracts, deciding disputes and changing protocol fees. If the admin is malicious, then all assets can be stolen by upgrading to a malicious contract which sends all funds to the admin. Therefore, I think it is very important that the admin is either a multi-sig wallet (which still has its problems) or even better a DAO. It may also be wise to separate admins for deciding disputes and all other operations, as deciding disputes requires faster responses than upgrading contracts.

Issue 3 Precision in checkPrecision() is independent of token decimals

The code for checkPrecision() is:

    function checkPrecision(uint256 _amount) internal pure {
        // Divide and multiply amount with 1000 should be equal to amount.
        // This ensures the amount is not too precise.
        require(
            ((_amount / 1000) * 1000) == _amount,
            "Project::Precision>=1000"
        );
    }

This, however, does not account for the token decimals being used e.g. USDC which is one of the currencies being used has 6 decimals while DAI has 18 decimals. Therefore 1000 is much more significant when using USDC then when using DAI

Consider changing the function to something like:

    function checkPrecision(uint256 _amount, uint256 decimals) internal pure {
        // Divide and multiply amount with 1000 should be equal to amount.
        // This ensures the amount is not too precise.
        uint256 precision = 10**(decimals-3);
        require(
            ((_amount / precision) * precision) == _amount,
            "Project::Precision Too Low"
        );
    }

Issue 4 __ReentrancyGuard_init() is not used

Several contracts including Community.sol, Disputes.sol, HomeFi.sol and Project.sol use ReentrancyGuardUpgradeable but do not initialize it. Consider, initializing it.

Issue 5 projectCost() checks can be easily bypassed

lendToProject() In Project.sol and toggleLendingNeeded() in Community.sol use projectCost() to make sure that the lending given is less than the total sum of costs of tasks required in the project. This can be easily bypassed by the builder by simply initializing new tasks with arbitrary costs and then simply setting the cost to zero later after a loan is given out. This makes the check completely defunct so it may be worthwhile to simply remove the use of this entire function

Issue 1 Use memory variables instead of state variables to save gas

In allocateFunds() from Project.sol, the function loops through all items in the dynamic array _changeOrderedTask using its length. However, the length is read from storage every loop, consider saving the length in memory and using that in the for loop

Additionally, this applies for looping up to taskCount

Consider rewriting to:

   function allocateFunds() public override {
        // Max amount out times this loop will run
        // This is to ensure the transaction do not run out of gas (max gas limit)
        uint256 _maxLoop = 50;

        // Difference of totalLent and totalAllocated is what can be used to allocate new tasks
        uint256 _costToAllocate = totalLent - totalAllocated;

        // Bool if max loop limit is exceeded
        bool _exceedLimit;

        // Local instance of lastAllocatedChangeOrderTask. To save gas.
        uint256 i = lastAllocatedChangeOrderTask;

        // Local instance of lastAllocatedTask. To save gas.
        uint256 j = lastAllocatedTask;

        uint256 _taskCount = taskCount;
        uint256 length = _changeOrderedTask.length;

        // Initialize empty array in which allocated tasks will be added.
        uint256[] memory _tasksAllocated = new uint256[](
            _taskCount - j + length - i
        );

        // Number of times a loop has run.
        uint256 _loopCount;

        /// CHANGE ORDERED TASK FUNDING ///
        

        // Any tasks added to _changeOrderedTask will be allocated first
        if (length > 0) {
            // Loop from lastAllocatedChangeOrderTask to _changeOrderedTask length (until _maxLoop)
            for (; i < length; i++) {
                // Local instance of task cost. To save gas.
                uint256 _taskCost = tasks[_changeOrderedTask[i]].cost;

                // If _maxLoop limit is reached then stop looping
                if (_loopCount >= _maxLoop) {
                    _exceedLimit = true;
                    break;
                }

                // If there is enough funds to allocate this task
                if (_costToAllocate >= _taskCost) {
                    // Reduce task cost from _costToAllocate
                    _costToAllocate -= _taskCost;

                    // Mark the task as allocated
                    tasks[_changeOrderedTask[i]].fundTask();

                    // Add task to _tasksAllocated array
                    _tasksAllocated[_loopCount] = _changeOrderedTask[i];

                    // Increment loop counter
                    _loopCount++;
                }
                // If there are not enough funds to allocate this task then stop looping
                else {
                    break;
                }
            }

            // If all the change ordered tasks are allocated, then delete
            // the changeOrderedTask array and reset lastAllocatedChangeOrderTask.
            if (i == length) {
                lastAllocatedChangeOrderTask = 0;
                delete _changeOrderedTask;
            }
            // Else store the last allocated change order task index.
            else {
                lastAllocatedChangeOrderTask = i;
            }
        }

        /// TASK FUNDING ///

        // If lastAllocatedTask is lesser than taskCount, that means there are un-allocated tasks
        if (j < _taskCount) {
            // Loop from lastAllocatedTask + 1 to taskCount (until _maxLoop)
            for (++j; j <= _taskCount; j++) {
                // Local instance of task cost. To save gas.
                uint256 _taskCost = tasks[j].cost;

                // If _maxLoop limit is reached then stop looping
                if (_loopCount >= _maxLoop) {
                    _exceedLimit = true;
                    break;
                }

                // If there is enough funds to allocate this task
                if (_costToAllocate >= _taskCost) {
                    // Reduce task cost from _costToAllocate
                    _costToAllocate -= _taskCost;

                    // Mark the task as allocated
                    tasks[j].fundTask();

                    // Add task to _tasksAllocated array
                    _tasksAllocated[_loopCount] = j;

                    // Increment loop counter
                    _loopCount++;
                }
                // If there are not enough funds to allocate this task then stop looping
                else {
                    break;
                }
            }

            // If all pending tasks are allocated store lastAllocatedTask equal to taskCount

            if (j > _taskCount) {
                lastAllocatedTask = _taskCount;
            }
            // If not all tasks are allocated store updated lastAllocatedTask
            else {
                lastAllocatedTask = --j;
            }
        }

        // If any tasks is allocated, then emit event
        if (_loopCount > 0) emit TaskAllocated(_tasksAllocated);

        // If allocation was incomplete, then emit event
        if (_exceedLimit) emit IncompleteAllocation();

        // Update totalAllocated with all allocations
        totalAllocated = totalLent - _costToAllocate;
    }

Using tests from repo: average gas used for allocateFunds() before fix: 63493 average gas used for allocateFunds() after fix: 63085

Average gas saved: 408