Rigor Protocol contest - Chom's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L705-L713 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L219-L263 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L185-L216 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L362-L383

Vulnerability details

Impact

projectCost may be reverted due to out of gas problem if having too many tasks. lendToProject and recoverTokens may always revert because of this.

If lendToProject always revert, community owner won't be able to lends fund to the published project. Tasks will never get funded.

If recoverTokens always revert, builder cannot recover any dust fund.

Proof of Concept

Start from projectCost function

    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;
        }
    }

if taskCount is too high (ex: >1000) running projectCost() may cost a lot of gas and hit the gas limit of Ethereum chain which cause the transaction to revert.

Next we will proof that we can add unlimited task (>1000 is possible).

    function addTasks(bytes calldata _data, bytes calldata _signature)
        external
        override
    {
        // If the sender is disputes contract, then do not check for signatures.
        if (_msgSender() != disputes) {
            // Check for required signatures
            checkSignature(_data, _signature);
        }

        // Decode params from _data
        (
            bytes[] memory _hash,
            uint256[] memory _taskCosts,
            uint256 _taskCount,
            address _projectAddress
        ) = abi.decode(_data, (bytes[], uint256[], uint256, address));

        // Revert if decoded taskCount is incorrect. This indicates wrong data.
        require(_taskCount == taskCount, "Project::!taskCount");

        // Revert if decoded project address does not match this contract. Indicating incorrect _data.
        require(_projectAddress == address(this), "Project::!projectAddress");

        // Revert if IPFS hash array length is not equal to task cost array length.
        uint256 _length = _hash.length;
        require(_length == _taskCosts.length, "Project::Lengths !match");

        // Loop over all the new tasks.
        for (uint256 i = 0; i < _length; i++) {
            // Increment local task counter.
            _taskCount += 1;

            // Check task cost precision. Revert if too precise.
            checkPrecision(_taskCosts[i]);

            // Initialize task.
            tasks[_taskCount].initialize(_taskCosts[i]);
        }

        // Update task counter equal to local task counter.
        taskCount = _taskCount;

        emit TasksAdded(_taskCosts, _hash);
    }

We can add 50 tasks each transaction for 21 times to has more than 1000 tasks which is possible as new task get pushed back into tasks mapping and add 50 tasks each never hit gas limit.

Then, we will discuss every functions that have impact in this

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

lendToProject use projectCost() which will always revert due to having too many tasks.

    function recoverTokens(address _tokenAddress) external override {
        /* If the token address is same as currency of this project,
            then first check if all tasks are complete */
        if (_tokenAddress == address(currency)) {
            // Iterate for each task and check if it is complete.
            uint256 _length = taskCount;
            for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {
                require(tasks[_taskID].getState() == 3, "Project::!Complete");
            }
        }
        ...

recoverTokens also loop all tasks which consume more gas than the limit in case _tokenAddress == address(currency)

This is a medium as it affect the availability of the community owner lending and also locked some dust fund.

Tools Used

Manual review

Recommended Mitigation Steps

Limit number of tasks that can be added.

Wrong comment

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L849-L850

// Burn _interestEarned amount wrapped token to lender
_wrappedToken.mint(_lender, _interestEarned);

It said "Burn _interestEarned amount wrapped token to lender" but actually "Mint _interestEarned amount wrapped token to lender"

Outdated solidity version

Almost every file use solidity 0.8.6; which is already outdated. Updating solidity version enable better optimization, newer security patch and new syntax.

Moreover, use fixed solidity version is an obstacle for any project integrating your contract since they might use different solidity version.

pragma solidity 0.8.6;

should be changed to

pragma solidity ^0.8.6;

EIP1271 is not supported

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/libraries/SignatureDecoder.sol#L20-L41

    function recoverKey(
        bytes32 messageHash,
        bytes memory messageSignatures,
        uint256 pos
    ) internal pure returns (address) {
        if (messageSignatures.length % 65 != 0) {
            return (address(0));
        }

        uint8 v;
        bytes32 r;
        bytes32 s;
        (v, r, s) = signatureSplit(messageSignatures, pos);

        // If the version is correct return the signer address
        if (v != 27 && v != 28) {
            return (address(0));
        } else {
            // solium-disable-next-line arg-overflow
            return ecrecover(toEthSignedMessageHash(messageHash), v, r, s);
        }
    }

Only ecrecover (ECDSA) is supported

Consider using openzeppelin's SignatureChecker if you want to support EIP1271

Missing address(0) check which is invalid result check on ecrecover

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L875-L892

    function checkSignatureValidity(
        address _address,
        bytes32 _hash,
        bytes memory _signature,
        uint256 _signatureIndex
    ) internal {
        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );
        require(
            _recoveredSignature == _address || approvedHashes[_address][_hash],
            "Project::invalid signature"
        );
        // delete from approvedHash
        delete approvedHashes[_address][_hash];
    }

If _recoveredSignature == address(0) and _address == address(0) it still accepted but in fact it shouldn't. Consider adding _recoveredSignature != address(0) check

    function checkSignatureValidity(
        address _address,
        bytes32 _hash,
        bytes memory _signature,
        uint256 _signatureIndex
    ) internal {
        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );
        require(
            _recoveredSignature != address(0) && (_recoveredSignature == _address || approvedHashes[_address][_hash]),
            "Project::invalid signature"
        );
        // delete from approvedHash
        delete approvedHashes[_address][_hash];
    }

Caching the length in for loops and increment in for loop postcondition can be made unchecked

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Caching the length in for loops:

1. if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first),
2. if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),
3. if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

for loop postcondition can be made unchecked Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant!

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L603

for (; i < _changeOrderedTask.length; i++) {

Can be optimized to

uint256 changeOrderedTaskLength = _changeOrderedTask.length;
for (uint256 i; i < changeOrderedTaskLength;) {
...
unchecked { ++i; }
}

Consider using ++i instead of i++

Currently every loops such as

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L603

are using i++ which is more expensive than ++i

for (; i < _changeOrderedTask.length; ++i) {

is better than

for (; i < _changeOrderedTask.length; i++) {

Consider using custom errors instead of revert strings

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deployment cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Any require statement in your code can be replaced with custom error for example,

require(_projectAddress == address(this), "Project::!projectAddress");

Can be replaced with

// declare error before contract declaration
error InvalidProject();

if (_projectAddress != address(this)) revert InvalidProject();

Consider adding unchecked

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L672

_loopCount++;

to

unchecked {
++_loopCount;
}

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L613-L630

                // 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;
                }

to

            unchecked {
                // 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;
                }
            }