Badger-Vested-Aura contest - Chom's results

Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

Instances

MyStrategy.sol:284:    // TODO: Hardcode claim.account = address(this)?
MyStrategy.sol:422:        // TODO: Too many SLOADs

Should use modifier instead of function call for role based access control

For example

    /// @dev Change Delegation to another address
    function manualSetDelegate(address delegate) external {
        _onlyGovernance();
        // Set delegate is enough as it will clear previous delegate automatically
        LOCKER.delegate(delegate);
    }

    ///@dev Should we check if the amount requested is more than what we can return on withdrawal?
    function setWithdrawalSafetyCheck(bool newWithdrawalSafetyCheck) external {
        _onlyGovernance();
        withdrawalSafetyCheck = newWithdrawalSafetyCheck;
    }

    ///@dev Should we processExpiredLocks during reinvest?
    function setProcessLocksOnReinvest(bool newProcessLocksOnReinvest) external {
        _onlyGovernance();
        processLocksOnReinvest = newProcessLocksOnReinvest;
    }

     ///@dev Change the contract that handles bribes
    function setBribesProcessor(IBribesProcessor newBribesProcessor) external {
        _onlyGovernance();
        bribesProcessor = newBribesProcessor;
    }

Should be converted to

    modifier onlyGovernance() {
        _onlyGovernance();
    }

    /// @dev Change Delegation to another address
    function manualSetDelegate(address delegate) external onlyGovernance {
        // Set delegate is enough as it will clear previous delegate automatically
        LOCKER.delegate(delegate);
    }

    ///@dev Should we check if the amount requested is more than what we can return on withdrawal?
    function setWithdrawalSafetyCheck(bool newWithdrawalSafetyCheck) external onlyGovernance {
        withdrawalSafetyCheck = newWithdrawalSafetyCheck;
    }

    ///@dev Should we processExpiredLocks during reinvest?
    function setProcessLocksOnReinvest(bool newProcessLocksOnReinvest) external onlyGovernance {
        processLocksOnReinvest = newProcessLocksOnReinvest;
    }

     ///@dev Change the contract that handles bribes
    function setBribesProcessor(IBribesProcessor newBribesProcessor) external onlyGovernance {
        bribesProcessor = newBribesProcessor;
    }

It is best practice to use modifier for role based access control instead of calling a private function.

Should use solidity 0.8 instead of 0.6 with SafeMathUpgradeable

It provide more readable, more security and better gas utilization if you use solidity 0.8.

_amount.mul(9_980).div(MAX_BPS) can be replaced with _amount * 9_980 / MAX_BPS in solidity 0.8 while providing better underflow and overflow guard

checkUpkeep should check for expired lock (a way is using try catch)

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L385-L388

    function checkUpkeep(bytes calldata checkData) external view returns (bool upkeepNeeded, bytes memory performData) {
        (, uint256 unlockable, ,) = LOCKER.lockedBalances(address(this));
        upkeepNeeded = unlockable > 0;
    }

Currently, it only check for whether locker has locked fund but don't know whether it can be unlocked or not.

Should be implemented this way

    function checkUpkeep(bytes calldata checkData) external view returns (bool upkeepNeeded, bytes memory performData) {
        try LOCKER.processExpiredLocks(false); { upkeepNeeded = true; } catch (bytes memory /*lowLevelData*/) { upkeepNeeded = false; }
    }

Caching the length in for loops

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

1. if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first),
2. if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),
3. if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L288-L343

    function claimBribesFromHiddenHand(IRewardDistributor hiddenHandDistributor, IRewardDistributor.Claim[] calldata _claims) external nonReentrant {
        _onlyGovernanceOrStrategist();
        require(address(bribesProcessor) != address(0), "Bribes processor not set");

        uint256 beforeVaultBalance = _getBalance();
        uint256 beforePricePerFullShare = _getPricePerFullShare();

        // Hidden hand uses BRIBE_VAULT address as a substitute for ETH
        address hhBribeVault = hiddenHandDistributor.BRIBE_VAULT();

        // Track token balances before bribes claim
        uint256[] memory beforeBalance = new uint256[](_claims.length);
        for (uint256 i = 0; i < _claims.length; i++) {
            (address token, , , ) = hiddenHandDistributor.rewards(_claims[i].identifier);
            if (token == hhBribeVault) {
                beforeBalance[i] = address(this).balance;
            } else {
                beforeBalance[i] = IERC20Upgradeable(token).balanceOf(address(this));
            }
        }

        // Claim bribes
        isClaimingBribes = true;
        hiddenHandDistributor.claim(_claims);
        isClaimingBribes = false;

        bool nonZeroDiff; // Cached value but also to check if we need to notifyProcessor
        // Ultimately it's proof of non-zero which is good enough

        for (uint256 i = 0; i < _claims.length; i++) {
            (address token, , , ) = hiddenHandDistributor.rewards(_claims[i].identifier);

            if (token == hhBribeVault) {
                // ETH
                uint256 difference = address(this).balance.sub(beforeBalance[i]);
                if (difference > 0) {
                    IWeth(address(WETH)).deposit{value: difference}();
                    nonZeroDiff = true;
                    _handleRewardTransfer(address(WETH), difference);
                }
            } else {
                uint256 difference = IERC20Upgradeable(token).balanceOf(address(this)).sub(beforeBalance[i]);
                if (difference > 0) {
                    nonZeroDiff = true;
                    _handleRewardTransfer(token, difference);
                }
            }
        }

        if (nonZeroDiff) {
            _notifyBribesProcessor();
        }

        require(beforeVaultBalance == _getBalance(), "Balance can't change");
        require(beforePricePerFullShare == _getPricePerFullShare(), "Ppfs can't change");
    }

Can be optimized to

    function claimBribesFromHiddenHand(IRewardDistributor hiddenHandDistributor, IRewardDistributor.Claim[] calldata _claims) external nonReentrant {
        _onlyGovernanceOrStrategist();
        require(address(bribesProcessor) != address(0), "Bribes processor not set");

        uint256 beforeVaultBalance = _getBalance();
        uint256 beforePricePerFullShare = _getPricePerFullShare();

        // Hidden hand uses BRIBE_VAULT address as a substitute for ETH
        address hhBribeVault = hiddenHandDistributor.BRIBE_VAULT();

        uint256 claimsLength = _claims.length;

        // Track token balances before bribes claim
        uint256[] memory beforeBalance = new uint256[](claimsLength);
        for (uint256 i = 0; i < claimsLength; i++) {
            (address token, , , ) = hiddenHandDistributor.rewards(_claims[i].identifier);
            if (token == hhBribeVault) {
                beforeBalance[i] = address(this).balance;
            } else {
                beforeBalance[i] = IERC20Upgradeable(token).balanceOf(address(this));
            }
        }

        // Claim bribes
        isClaimingBribes = true;
        hiddenHandDistributor.claim(_claims);
        isClaimingBribes = false;

        bool nonZeroDiff; // Cached value but also to check if we need to notifyProcessor
        // Ultimately it's proof of non-zero which is good enough

        for (uint256 i = 0; i < claimsLength; i++) {
            (address token, , , ) = hiddenHandDistributor.rewards(_claims[i].identifier);

            if (token == hhBribeVault) {
                // ETH
                uint256 difference = address(this).balance.sub(beforeBalance[i]);
                if (difference > 0) {
                    IWeth(address(WETH)).deposit{value: difference}();
                    nonZeroDiff = true;
                    _handleRewardTransfer(address(WETH), difference);
                }
            } else {
                uint256 difference = IERC20Upgradeable(token).balanceOf(address(this)).sub(beforeBalance[i]);
                if (difference > 0) {
                    nonZeroDiff = true;
                    _handleRewardTransfer(token, difference);
                }
            }
        }

        if (nonZeroDiff) {
            _notifyBribesProcessor();
        }

        require(beforeVaultBalance == _getBalance(), "Balance can't change");
        require(beforePricePerFullShare == _getPricePerFullShare(), "Ppfs can't change");
    }

Should use solidity ^0.8.4 instead of 0.6 with SafeMathUpgradeable

It provide more readable, more security and better gas utilization if you use solidity 0.8.

_amount.mul(9_980).div(MAX_BPS) can be replaced with _amount * 9_980 / MAX_BPS in solidity 0.8 while providing better gas cost, underflow and overflow guard.

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Safemath by default from 0.8.0 (can be more gas efficient than some library-based safemath.)

Consider using custom errors instead of revert strings (Upgrade to solidity ^0.8.4 first)

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deployment cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Any require statement in your code can be replaced with custom error for example,

require(address(bribesProcessor) != address(0), "Bribes processor not set");

Can be replaced with

// declare error before contract declaration
error BribesNotSet();

if (address(bribesProcessor) == address(0)) revert BribesNotSet();