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

1 stars 1 forks source link

Gas Optimizations #204

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Using Clone instead of CREATE2

CREATE2 is consuming a lot of gas while Clone consume 10 times less gas than CREATE2

The WrappedFCash contract already used initialize pattern, so it is already compatible with Clone.

Clone is smaller than Beacon.

Read more: https://blog.logrocket.com/cloning-solidity-smart-contracts-factory-pattern/

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol#L26-L37

    function deployWrapper(uint16 currencyId, uint40 maturity) external returns (address) {
        address _computedWrapper = computeAddress(currencyId, maturity);

        if (Address.isContract(_computedWrapper)) {
            // If wrapper has already been deployed then just return it's address
            return _computedWrapper;
        } else {
            address wrapper = Create2.deploy(0, SALT, _getByteCode(currencyId, maturity));
            emit WrapperDeployed(currencyId, maturity, wrapper);
            return wrapper;
        }
    }

This part use Create2 to deploy Beacon, can be optimized to use Clone.

You may use clone with any kind of upgradable proxy to achieve upgradability.

Use solidity custom error instead of revert message

It not only provide better gas optimization but also reduce contract size

https://blog.soliditylang.org/2021/04/21/custom-errors/

Applied to all area with require(...)

require(cashGroup.maxMarketIndex > 0, "Invalid currency");

        require(
            DateTime.isValidMaturity(cashGroup.maxMarketIndex, maturity, block.timestamp),
            "Invalid maturity"
        );

require(pvExternal >= 0);

require(!hasMatured(), "fCash matured");

... any many more ...

Can be written like this

// Before contract definition
error Matured();

// Inside a function
if (hasMatured()) {
    revert Matured();
}

Duplicated function call on convertToShares

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L52-L61

    function convertToShares(uint256 assets) public view override returns (uint256 shares) {
        uint256 supply = totalSupply();
        if (supply == 0) {
            // Scales assets by the value of a single unit of fCash
            uint256 unitfCashValue = _getPresentValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
            return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
        }

        return (assets * totalSupply()) / totalAssets();
    }

Noticed that totalSupply() is called twice. Can be optimized to

    function convertToShares(uint256 assets) public view override returns (uint256 shares) {
        uint256 supply = totalSupply();
        if (supply == 0) {
            // Scales assets by the value of a single unit of fCash
            uint256 unitfCashValue = _getPresentValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
            return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
        }

        return (assets * supply) / totalAssets();
    }