Rigor Protocol contest - cryptonue's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L185-L197

Vulnerability details

Impact

There are some functions that the admin can call to brick various parts of the system

Even if the owner is benevolent the fact that there is a rug vector available may negatively impact the protocol’s reputation.

Proof of Concept

Admin can set LenderFee to an amount that can break / rug the protocol.

    /// @inheritdoc IHomeFi
    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);
    }

Recommended Mitigation Steps

Set maximum and minimum amount (of the LenderFee) allowed

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Community.sol#L206-L282

Vulnerability details

Impact

When publishing a project, there is still possibility the project doesn't have any task or 0 budget.

Proof of Concept

According to contest guideline, there is an information says

"Note that you cannot submit a project with no total budget. Therefore it requires at least one task with a budget > 0."

Meanwhile, on publishProject() in Community.sol, there is no check of this condition.

Recommended Mitigation Steps

Add a new require which will check if the first task (which is at index 1), its cost is > 0.

  // Local instance of variables. For saving gas.
  IProject _projectInstance = IProject(_project);
  ...

  // Revert if project doesn't have one task with budget > 0
  require(_projectInstance.tasks[1].cost > 0, "First task > 0");

USE OF BLOCK.TIMESTAMP

block.timestamp is being used several times inside Community.sol file.

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

INTERNAL FUNCTIONS ONLY CALLED ONCE CAN BE INLINED TO SAVE GAS

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

example internal function mintNFT() inside HomeFi.sol

USE A MORE RECENT VERSION OF SOLIDITY and OpenZeppelin Library

current solidity version is 0.8.15 meanwhile project is using 0.8.6. OpenZeppelin library currently 4.7.2 while the project is using 4.4.2. It's best to use latest version because there are some features improvement and security patch.

USE CUSTOM ERRORS RATHER THAN REVERT()/REQUIRE() STRINGS TO SAVE GAS

Rigor is using solidity version 0.8.6, while custom errors are available from solidity version 0.8.4, so it's suitable to use custom error. Custom errors save ~50 gas each time they’re hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

<ARRAY>.LENGTH SHOULD NOT BE LOOKED UP IN EVERY LOOP OF A FOR-LOOP

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

example loop _changeOrderedTask.length inside Project.sol

++I COSTS LESS GAS THAN I++, ESPECIALLY WHEN IT’S USED IN FOR-LOOPS (--I/I-- TOO)

Saves 6 gas per loop

example loop inside Community.sol, Project.sol, HomeFiProxy.sol, Tasks.sol

for (uint256 i = 0; i < _length; i++) 

SPLITTING REQUIRE() STATEMENTS THAT USE && SAVES GAS

See (this issue)[https://github.com/code-423n4/2022-01-xdefi-findings/issues/128] which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper.

this issue exist inside Community.sol, Disputes.sol

require(
    _lendingNeeded >= _communityProject.totalLent &&
        _lendingNeeded <= IProject(_project).projectCost(),
    "Community::invalid lending"
);

USING PRIVATE RATHER THAN PUBLIC FOR CONSTANTS, SAVES GAS

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

example issue in Project.sol

uint256 public constant override VERSION = 25000;