Gas Optimizations

[code423n4]

Executive Summary

Codebase is very well thought-out, because we cannot use immutables, I highly recommend hardcoding constants to save gas.

Below are listed gas savings suggestions, sorted by impact and ease of use.

Unless a benchmark is shown, below are estimates based on known EVM OpCodes

Biggest Gas Save - Hardcode Unchangeable Variables - 2.1k+ per variable - Up to 10k+ per transaction

All proxies are deployed from a logic via the HomeFiProxy.
Because of the upgradeable pattern and to make testing generalizable, addresses for:

• tokenCurrency1
• tokenCurrency2
• tokenCurrency3
• homeFi

Are initialized and saved into storage in multiple contracts.

This is a way to make the code more re-usable, especially in preparation for Production.

However:

• Your contracts are upgradeable (reduced risk of bricking)
• Those variables will be set at initialization and won't be changed.

I recommend hardcoding the values at the top of the file for the actual deployments.

As a general rule, if the variable is "unchangeable" (no setter), and the contract is upgradeable, then you're better off hardcoding to save the end user thousands of gas.

From my experience, you'll be saving more gas from hardcoding those few variables than most other techniques

Slightly Tweak ProjectDetails Struct for Packing - save 2 Slots = up to 15k * 2 = 30k gas per debt operation - 4.2k+ per read

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/interfaces/ICommunity.sol#L18-L32

We know: apr is meant to be in 1000 (I recommend bps, but either way it's a small number)
lastTimestamp can be represented with a uint64 and that's going to cover 1.8e19 seconds which is plenty
publishFeePaid is a boolean, which occupies only 1 bit

We can refactor the struct to

    struct ProjectDetails {
        // Percentage of interest rate of project.
        // The percentage must be multiplied with 10. Lowest being 0.1%.
        
        uint256 lendingNeeded; // total lending requirement from the community
        // Total amount that has been transferred to a project from a community.
        // totalLent = lentAmount - lenderFee - anyRepayment.
        uint256 totalLent;
        uint256 publishFee; // fee required to be paid to request funds from community
        /// @dev Slot 3 packs 3 variables
        uint64 apr;
        uint64 lastTimestamp; // timestamp when last lending / repayment was made
        bool publishFeePaid; // boolean indicating if publish fee is paid
        /// @dev end slot 4
        uint256 lentAmount; // current principal lent to project (needs to be repaid by project's builder)
        uint256 interest; // total accrued interest on `lentAmount`
        
    }

We could be more aggressive, perhaps by removing the decimals of tokens and further reducing each variable to a uint128 (way more than enough), however the suggestion above should be very easy to implement and extremely effective

Similar packing for CommunityStruct - 15k gas * 2 instances on first use

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/interfaces/ICommunity.sol#L35-L43

We can apply clever packing strategies to Save the cost of writing to an empty Slot on 2 separate instances

uint48 allows way more than billions of people to be member of the community, hence the finding has no loss of functionality. This applies to publishNonce as well

CommunityStruct

    struct CommunityStruct {

        /// @dev Pack `owner` with `memberCount`
        address owner; // owner of the community
        uint48 memberCount; // count of members in the community
        
        /// @dev Pack `currency` with `publishNonce`
        IDebtToken currency; // token currency of the community
        uint48 publishNonce; // unique nonce counter. Used for unique signature at `publishProject`.

These 2 packings should offer no-loss of functionality (and no reasonable risk of overflow), while providing gas savings of up to 15k per pair (30k combined) as the slot will never go to 0, meaning any subsequent change will cost 5k instead of 20k

Task Library Packing Optimization - up to 15k gas per tx

Alerts is a mapping that is used mostly as a Finite State Machine that toggles certain states.

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L10-L17

struct Task {
    // Metadata //
    uint256 cost; // Cost of task
    address subcontractor; // Subcontractor of task
    // Lifecycle //
    TaskStatus state; // Status of task
    mapping(uint256 => bool) alerts; // Alerts of task
}

We can see the practical usage here in getAlerts

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

    /// @inheritdoc IProject
    function getAlerts(uint256 _taskID)
        public
        view
        override
        returns (bool[3] memory _alerts)
    {
        return tasks[_taskID].getAlerts();
    }

Effectively you are using a mapping, that uses 3 separate SLOADs (20k on first write per slot, or 5k on subsequent change) for keeping track of the state.

A basic example would be to create a Struct with all possible combinations, an alternative (more complex), and a more complete solution would require packing booleans into a hex string.

See this excellent writeup by iamdefinitelyahuman:
https://ethereum.stackexchange.com/a/77102/76762

Remove DebtToken for additional Gas Savings - 5k / 15k gas per transaction

DebtToken is used in a way where:

• Lending and Claiming Interest increases the amount of token
• Repaying debt reduces the balance

Meaning that the exact amount of the token is always derivable by _communityProject.lentAmount and _communityProject.interest.

For those reasons, the token is an expensive cosmetic decision and I'd recommend removing it to save a lot of overhead.

Further considerations / Compromise

To keep the token, while removing the extra overhead, we could rewrite the token to simply check the sum of _communityProject.lentAmount + _communityProject.interest for balanceOf(lender)

Meaning, while I'd recommend a complete removal, you could still retain the functionality but save the 15k gas on payment operations.

Avoid Needless extra SLOAD if you can get an early True in assertMember - Up to 4.4k gas 1/3 of the times

In assertMember since you've already calculated _sc, it would save potentially 2 CALL (100 each) and 2 SLOADS (2.1k each) to compare that first.

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L180-L182

      bool _result = _projectInstance.builder() == _address ||
            _projectInstance.contractor() == _address ||
            _sc == _address;

Change to

      bool _result =_sc == _address || // Put this comparison at top to save gas in optimistic case, no change in other cases
             _projectInstance.builder() == _address ||
            _projectInstance.contractor() == _address;
            

Minimize SLOADS - 100 gas per instance

Because your contracts are upgradeable, most variables are going to be read from storage.

For that reason, when re-reading the same value form storage, even once, you are going to save gas by caching it in memory

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/ProjectFactory.sol#L84-L90

        require(_msgSender() == homeFi, "PF::!HomeFiContract");

        // Create clone of Project implementation
        _clone = ClonesUpgradeable.clone(underlying);

        // Initialize project
        Project(_clone).initialize(_currency, _sender, homeFi); // NOTE: `homeFi` second SLOAD

Change to use a Memory Variable

        address cachedHomeFi = homeFi; // 3 gas + 2.1k (SLOAD)
        require(_msgSender() == cachedHomeFi, "PF::!HomeFiContract"); // 3 gas

        // Create clone of Project implementation
        _clone = ClonesUpgradeable.clone(underlying);

        // Initialize project
        Project(_clone).initialize(_currency, _sender, cachedHomeFi); // 3 gas

The savings will be an additional 100gas for any additional SLOAD removed

Local Variable for Casting is more expensive than inline casting

In Community.publishProject you store _projectInstance for IProject(_project);

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

        IProject _projectInstance = IProject(_project);

What's happening underneath is you're allocating a space in memory (MSTORE, 3 gas) for the value of _project.

All the casting to IProject is an abstraction offered by the solidity compiler.

You'd save 3 gas in each instance by simply inlining the cast instead of storing the casted value in memory.

E.g.
IProject(_project).currency() == _community.currency,

Suggestion

Delete _projectInstance, and inline cast when necessary for minor gas savings

Optimized Loops - Unchecked Pre-Increment - 25 / 80 per iteration

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L136-L137

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

Change to

        for (uint256 i = 0; i < _length; ) {
          
          // At end
          { unchecked ++i; }
        }

This change will save at least 25 gas per iteration and most of the times up to 80