Gas Optimizations

[code423n4]

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S] Duplicated assert statements

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/curve-v2/Swap.vy#L666-L671

@external
@nonreentrant('lock')
def add_liquidity(amounts: uint256[N_COINS], min_mint_amount: uint256) -> (uint256):
    assert msg.sender == self.amm, 'VAMM: OnlyAMM'
    assert not self.is_killed  # dev: the pool is killed
    assert msg.sender == self.amm

L669 and L671 are duplicated.

[S] Outdated versions of OpenZeppelin library

Outdated versions of OpenZeppelin library are used.

It's a best practice to use the latest version of libraries.

New versions of OpenZeppelin libraries can be more gas effeicant.

For exmaple:

ERC20Upgradeable.sol in @openzeppelin/contracts-upgradeable@4.3.2:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v4.3.2/contracts/token/ERC20/ERC20Upgradeable.sol#L155-L169

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public virtual override returns (bool) {
        _transfer(sender, recipient, amount);

        uint256 currentAllowance = _allowances[sender][_msgSender()];
        require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance");
        unchecked {
            _approve(sender, _msgSender(), currentAllowance - amount);
        }

        return true;
    }

A gas optimization upgrade has been added to @openzeppelin/contracts@4.5.0:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v4.5.0/contracts/token/ERC20/ERC20Upgradeable.sol#L163-L172

    function transferFrom(
        address from,
        address to,
        uint256 amount
    ) public virtual override returns (bool) {
        address spender = _msgSender();
        _spendAllowance(from, spender, amount);
        _transfer(from, to, amount);
        return true;
    }

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v4.5.0/contracts/token/ERC20/ERC20Upgradeable.sol#L335-L347

    function _spendAllowance(
        address owner,
        address spender,
        uint256 amount
    ) internal virtual {
        uint256 currentAllowance = allowance(owner, spender);
        if (currentAllowance != type(uint256).max) {
            require(currentAllowance >= amount, "ERC20: insufficient allowance");
            unchecked {
                _approve(owner, spender, currentAllowance - amount);
            }
        }
    }

• reduce allowance before triggering transfer. (#3056)
• do not update allowance on transferFrom when allowance is type(uint256).max. (#3085)
• cache _msgSender (#3167)

[S] Avoid unnecessary storage reads in for loops can save gas

For the storage variables that will be accessed multiple times, especially in loops, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).

For example:

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/VUSD.sol#L53-L67

    function processWithdrawals() external {
        uint reserve = reserveToken.balanceOf(address(this));
        require(reserve >= withdrawals[start].amount, 'Cannot process withdrawals at this time: Not enough balance');
        uint i = start;
        while (i < withdrawals.length && (i - start) <= maxWithdrawalProcesses) {
            Withdrawal memory withdrawal = withdrawals[i];
            if (reserve < withdrawal.amount) {
                break;
            }
            reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount);
            reserve -= withdrawal.amount;
            i += 1;
        }
        start = i;
    }

start and maxWithdrawalProcesses can be cached.

[M] Adding unchecked directive can save gas

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L572-L579

    function _transferInVusd(address from, uint amount) internal {
        IERC20(address(vusd)).safeTransferFrom(from, address(this), amount);
        if (credit > 0) {
            uint toBurn = Math.min(vusd.balanceOf(address(this)), credit);
            credit -= toBurn;
            vusd.burn(toBurn);
        }
    }

credit -= toBurn will never overflow.

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L581-L593

    function _transferOutVusd(address recipient, uint amount) internal {
        uint bal = vusd.balanceOf(address(this));
        if (bal < amount) {
            // Say there are 2 traders, Alice and Bob.
            // Alice has a profitable position and realizes their PnL in form of vusd margin.
            // But bob has not yet realized their -ve PnL.
            // In that case we'll take a credit from vusd contract, which will eventually be returned when Bob pays their debt back.
            uint _credit = amount - bal;
            credit += _credit;
            vusd.mint(address(this), _credit);
        }
        IERC20(address(vusd)).safeTransfer(recipient, amount);
    }

uint _credit = amount - bal will never overflow.

[S] Using immutable variable can save gas

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccountHelper.sol#L15-L26

    IMarginAccount marginAccount;
    VUSD vusd;
    IERC20 public reserveToken;

    constructor(address _marginAccount, address _vusd) {
        marginAccount = IMarginAccount(_marginAccount);
        vusd = VUSD(_vusd);
        reserveToken = vusd.reserveToken();

        reserveToken.safeApprove(address(_vusd), type(uint).max);
        IERC20(_vusd).safeApprove(address(_marginAccount), type(uint).max);
    }

Considering that marginAccount, vusd and reserveToken will never change, changing them to immutable variables instead of storages variable can save gas.

[atvanguard]

Good report but a duplicate of several other issues.

[moose-code]

Really like the S M N report nomenclature. Very true that gas optimisations are not one size fits all. I mean, we could write the whole thing in opcodes but readability is destroyed.