search

code-423n4 / 2022-06-notional-coop-findings

1 stars 1 forks source link

Gas Optimizations #175

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

GAS

1. Title: Using delete statement to set mapping value to false

Occurrences: https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/Controller.sol#L180 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/Controller.sol#L210 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/Controller.sol#L240 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/Controller.sol#L279

Instead of set the mapping value to false, using delete statement to set it to default value can save gas. Change to:

        isSet[_setToken] = false;

2. Title: Using prefix increment

Occurrences: https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/Controller.sol#L135-L149 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/IntegrationRegistry.sol#L112-L119 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L97-L99 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetValuer.sol#L89

Using prefix increment the way more effective for gas optimization in for() loop Change to:

for (uint256 i; i < _factories.length; ++i) {
            require(_factories[i] != address(0), "Zero address submitted.");
            isFactory[_factories[i]] = true;
        }

3. Title: Using != operator

Occurrences: https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/IntegrationRegistry.sol#L108 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L489 https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L23

Using != instead of < to validate than uint value is not 0 is more effective

4. Title: Using calldata to store array parameter

Occurrences: https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/IntegrationRegistry.sol#L98-L100 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L82-L85 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetTokenCreator.sol#L67-L69

Using calldata as a pointer to store array in the function can save gas Change to:

function batchAddIntegration(
        address[] calldata _modules,
        string[] calldata _names,
        address[] calldata _adapters
    )
        external
        onlyOwner
    {

5. Title: Emit IntegrationRemoved earlier in removeIntegration() function https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/IntegrationRegistry.sol#L187-L194

Emitting the event earlier before deleting the integrations mapping value (L192) can save gas by not doing MSTORE Change to:

    function removeIntegration(address _module, string memory _name) external onlyOwner {
        bytes32 hashedName = _nameHash(_name);
        require(integrations[_module][hashedName] != address(0), "Integration does not exist.");

    emit IntegrationRemoved(_module, integrations[_module][hashedName], _name);// @audit-info: Avoiding MSTORE by calling it before deletion below

        delete integrations[_module][hashedName];
    }

6. Title: Efficiency if() in getPrice() https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L128-L134

By placing second if() statement (L132) and require() inside first if() (L128) can prevent unnecessary MLOAD if priceFound value is already set true at L126 Change to:

if (!priceFound) { //@audit-info: The function will just returning first price value with no validation  if priceFound isTrue
            (priceFound, price) = _getPriceFromMasterQuote(_assetOne, _assetTwo);

    if (!priceFound) {
            (priceFound, price) = _getPriceFromAdapters(_assetOne, _assetTwo);
        require(priceFound, "PriceOracle.getPrice: Price not found."); //@audit-info: Validate that the final value is true
        }
}

7. Title: Returning default value in function

Occurrences: https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L277 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L311 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L342

By removing return statement at L277, we can save 14 gas (if hasDirectOracle and hasInverseOracle is false). It is more efficient because by not returning any value, the function will just returning default var value (uint(0) and bool(false)) without set returning value with (false, 0)

8. Title: Packing var in SetToken.sol https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L110-L141

By placing isLocked var next to any address var, we can pack those vars in 1 single slot. Address has 20 bytes size and bool 1 bytes size. The max bytes size storage in solidity can store is 32 bytes. Change to:

    IController public controller;

    // The manager has the privelege to add modules, remove, and set a new manager
    address public manager;

    // A module that has locked other modules from privileged functionality, typically required
    // for multi-block module actions such as auctions
    address public locker;

    // without interruption
    bool public isLocked; //@audit-info: Place here

    // List of initialized Modules; Modules extend the functionality of SetTokens
    address[] public modules;

    // Modules are initialized from NONE -> PENDING -> INITIALIZED through the
    // addModule (called by manager) and initialize  (called by module) functions
    mapping(address => ISetToken.ModuleState) public moduleStates;

    // When locked, only the locker (a module) can call privileged functionality
    // Typically utilized if a module (e.g. Auction) needs multiple transactions to complete an action

    // List of components
    address[] public components;

    // Mapping that stores all Default and External position information for a given component.
    // Position quantities are represented as virtual units; Default positions are on the top-level,
    // while external positions are stored in a module array and accessed through its externalPositions mapping
    mapping(address => ISetToken.ComponentPosition) private componentPositions;

    // The multiplier applied to the virtual position unit to achieve the real/actual unit.
    // This multiplier is used for efficiently modifying the entire position units (e.g. streaming fee)
    int256 public positionMultiplier;

9. Title: Unnecessary multiple return statement https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L207-L211

We can just remove return statement at L211 Change to:

    function invoke(
        address _target,
        uint256 _value,
        bytes calldata _data
    )
        external
        onlyModule
        whenLockedOnlyLocker
        returns (bytes memory _returnValue)
    {
        _returnValue = _target.functionCallWithValue(_data, _value);//@audit-info:The function has returned the value here

        emit Invoked(_target, _value, _data, _returnValue);

    //@audit-info: Removed return statement at L211

    }

10. Title: Unnecessary else statement

Occurrences: https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashBase.sol#L130-L132 https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L98 https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L118

The if() condition (if the condition == true) in getUnderlyingToken() function has returned the value (so the line below it won't be executed). By removing else statement we can save gas

Change to:

        if (asset.tokenType == TokenType.NonMintable) {
            // In this case the asset token is the underlying
            return (IERC20(asset.tokenAddress), asset.decimals);
        }

        return (IERC20(underlying.tokenAddress), underlying.decimals);

11. Title: Cheaper way to returning in convertToShares()

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L60

Instead of running totalSupply(), using supply is cheaper

Change to:

        return (assets * supply) / totalAssets();

And also use just one method to returning value (the shares var is useless since the function returning the value directly / not using shares to store the value)

Recommend to:

//@audit-info: Remove shares var
function convertToShares(uint256 assets) public view override returns (uint256)

12. Title: Unnecessary hasInverseOracle MSTORE

https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L262

Currently hasDirectOracle merely called once in the function. Checking that address(directOracle) =! address(0) inside the if condition can save gas by avoiding MSTORE. It also quite clear (for readability) that validate address var != address(0) is to ensure that the var is already set Change to:

        IOracle directOracle = oracles[_assetOne][_assetTwo];
        //@audit-info: remove var declaration

        // Check asset1 -> asset 2. If exists, then return value
        if (address(directOracle) != address(0)) { //@audit-info: validate directly here
            return (true, directOracle.read());
        }

Same thing can be applied at L262 (hasDirectOracle)

13. Title: Make string >= 32 bytes

Occurrences: https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/Controller.sol#L294 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/Controller.sol#L311 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/IntegrationRegistry.sol#L109-L110 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/IntegrationRegistry.sol#L168-L169 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L136 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L200 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L215

Strings are broken into 32 byte chunks for operations. Revert error strings over 32 bytes therefore consume extra gas than shorter strings

14. Title: Unnecessary onlyModule modifier

https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L352

The function has checked that the msg.sender == locker. locker is set in lock function which has onlyModule modifier (means that only module who can fill locker var).

ckoopmann commented 2 years ago

On the Index / SetProtocol side this report seems to excusively reference contracts that are out of scope of this contest.