Backd contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/pool/EthPool.sol#L30

Vulnerability details

Since the introduction of transfer(), it has typically been recommended by the security community because it helps guard against reentrancy attacks. This guidance made sense under the assumption that gas costs wouldn’t change. It's now recommended that transfer() and send() be avoided, as gas costs can and will change and reentrancy guard is more commonly used.

Any smart contract that uses transfer() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.

Impact

• EOA users may not be able to call LiquidityPool.redeem()
  • LiquidityPool.redeem() L564 will call _rebalanceVault(redeemUnderlying);
    • LiquidityPool._rebalanceVault() L754 will call _doTransferOut(payable(address(vault)), excessDeposits);
      • _doTransferOut(address payable to, uint256 amount) L30 will call to.transfer(amount);. And _doTransferOut(payable(address(vault)), excessDeposits); will call vault with 2300 gas limit
      • vault is Upgradeable
        • the Proxy contract's receive() external payable will use delegatecall to call the implementation contract
        • However, cold DELEGATECALL requires 2600 of gas, exceeds the gas limit of address.transfer(), which is 2300, therefore, the tx will revert.
• smart contract may not be able to call LiquidityPool.redeem()
  • LiquidityPool.redeem() L567 will call _doTransferOut(payable(msg.sender), redeemUnderlying);
    • _doTransferOut(address payable to, uint256 amount) L30 will call to.transfer(amount);. And _doTransferOut(payable(msg.sender), redeemUnderlying); will call msg.sender with 2300 gas limit
    • When msg.sender is a smart contract, it's highly likely the 2300 gas limit is not enough, therefore the tx will revert.
• VaultReserve.withdraw() may revert
  • VaultReserve.withdraw() L81 will call payable(msg.sender).transfer(amount);
    • payable(msg.sender).transfer(amount); will call msg.sender with 2300 gas limit
    • msg.sender aka Vault (as VaultReserve.withdraw() is onlyVault)
    • vault is Upgradeable
    • the Proxy contract's receive() external payable will use delegatecall to call the implementation contract, However, cold DELEGATECALL requires 2600 of gas, exceeds the gas limit of address.transfer(), which is 2300, therefore, the tx will revert.

Recommendation

It's recommended to stop using transfer() and switch to using call() instead.

See: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v4.5.2/contracts/utils/AddressUpgradeable.sol#L44-L65

Consider using OpenZeppelin's AddressUpgradeable#sendValue().

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/pool/EthPool.sol#L29-L31

    function _doTransferOut(address payable to, uint256 amount) internal override {
        to.transfer(amount);
    }

Can be changed to:

    function _doTransferOut(address payable to, uint256 amount) internal override {
          AddressUpgradeable.sendValue(to, amount);
    }

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/vault/VaultReserve.sol#L72-L87

    function withdraw(address token, uint256 amount) external override onlyVault returns (bool) {
        require(canWithdraw(msg.sender), Error.RESERVE_ACCESS_EXCEEDED);
        uint256 accountBalance = _balances[msg.sender][token];
        require(accountBalance >= amount, Error.INSUFFICIENT_BALANCE);

        _balances[msg.sender][token] -= amount;
        _lastWithdrawal[msg.sender] = block.timestamp;

        if (token == address(0)) {
            payable(msg.sender).transfer(amount);
        } else {
            IERC20(token).safeTransfer(msg.sender, amount);
        }
        emit Withdraw(msg.sender, token, amount);
        return true;
    }

Can be changed to:

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/vault/VaultReserve.sol#L72-L87

    function withdraw(address token, uint256 amount) external override onlyVault returns (bool) {
        require(canWithdraw(msg.sender), Error.RESERVE_ACCESS_EXCEEDED);
        uint256 accountBalance = _balances[msg.sender][token];
        require(accountBalance >= amount, Error.INSUFFICIENT_BALANCE);

        _balances[msg.sender][token] -= amount;
        _lastWithdrawal[msg.sender] = block.timestamp;

        if (token == address(0)) {
            AddressUpgradeable.sendValue(payable(msg.sender), amount);
        } else {
            IERC20(token).safeTransfer(msg.sender, amount);
        }
        emit Withdraw(msg.sender, token, amount);
        return true;
    }

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L63-L66

Vulnerability details

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L63-L66

    function _ethPrice() private view returns (int256) {
        (, int256 answer, , , ) = _ethOracle.latestRoundData();
        return answer;
    }

On ChainlinkUsdWrapper.sol, we are using latestRoundData, but there is no check if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation:

• https://docs.chain.link/docs/historical-price-data/#historical-rounds
• https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Recommendation

Consider adding missing checks for stale data.

For example:

(uint80 roundID, int256 answer, , uint256 updatedAt, uint80 answeredInRound) = _ethOracle.latestRoundData();
require(answer > 0, "Chainlink price <= 0"); 
require(answeredInRound >= roundID, "Stale price");
require(updatedAt != 0, "Round not complete");

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/strategies/StrategySwapper.sol#L287-L289

Vulnerability details

In swapAllForWeth() and swapAllWethForToken, when using tokens with decimals larger than 18, the txs will revert due to underflow at _decimalMultiplier.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/strategies/StrategySwapper.sol#L287-L289

function _decimalMultiplier(address token_) internal view returns (uint256) {
    return 10**(18 - IERC20Full(token_).decimals());
}

Lines of code

https://github.com/compound-finance/compound-protocol/blob/v2.8.1/contracts/CEther.sol#L44-L47

Vulnerability details

https://github.com/compound-finance/compound-protocol/blob/v2.8.1/contracts/CEther.sol#L44-L47

function mint() external payable {
    (uint err,) = mintInternal(msg.value);
    requireNoError(err, "mint failed");
}

mint() for native cToken (CEther) does not have any parameters, as the Function Selector is based on the function name with the parenthesised list of parameter types, when you add a nonexisting parameter, the Function Selector will be incorrect.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/interfaces/vendor/CTokenInterfaces.sol#L316

    function mint(uint256 mintAmount) external payable virtual returns (uint256);

The current implementation uses the same CToken interface for both CEther and CErc20 in topUp(), and function mint(uint256 mintAmount) is a nonexisting function for CEther.

As a result, the native token topUp() always revert.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L57-L70

    CToken ctoken = cTokenRegistry.fetchCToken(underlying);
    uint256 initialTokens = ctoken.balanceOf(address(this));

    address addr = account.addr();

    if (repayDebt) {
        amount -= _repayAnyDebt(addr, underlying, amount, ctoken);
        if (amount == 0) return true;
    }

    uint256 err;
    if (underlying == address(0)) {
        err = ctoken.mint{value: amount}(amount);
    }

See also:

• Compound's cToken mint doc: https://compound.finance/docs/ctokens#mint

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/interfaces/vendor/CTokenInterfaces.sol#L355-L358

Vulnerability details

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/interfaces/vendor/CTokenInterfaces.sol#L355-L358

function repayBorrowBehalf(address borrower, uint256 repayAmount)
        external
        payable
        returns (uint256);

repayBorrowBehalf() for native cToken (CEther) will return nothing, while the current CEthInterface interface defines the returns as (uint256).

As a result, ether.repayBorrowBehalf() will always revert

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L117-L118

    CEther cether = CEther(address(ctoken));
    err = cether.repayBorrowBehalf{value: debt}(account);

Ref:

• Compound cToken Repay Borrow Behalf doc: https://compound.finance/docs/ctokens#repay-borrow-behalf
• Compound CEther.repayBorrowBehalf() https://github.com/compound-finance/compound-protocol/blob/v2.8.1/contracts/CEther.sol#L92-L95
• Compound CErc20.repayBorrowBehalf() https://github.com/compound-finance/compound-protocol/blob/v2.8.1/contracts/CErc20.sol#L94-L97

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/interfaces/vendor/CTokenInterfaces.sol#L345

Vulnerability details

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/interfaces/vendor/CTokenInterfaces.sol#L345

 function mint() external payable returns (uint256);

mint() for native cToken (CEther) will return nothing, while the current CEthInterface interface defines the returns as (uint256).

In the current implementation, the interface for CToken is used for both CEther and CErc20.

As a result, the transaction will revert with the error: function returned an unexpected amount of data when topUp() with the native token (ETH).

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L57-L70

    CToken ctoken = cTokenRegistry.fetchCToken(underlying);
    uint256 initialTokens = ctoken.balanceOf(address(this));

    address addr = account.addr();

    if (repayDebt) {
        amount -= _repayAnyDebt(addr, underlying, amount, ctoken);
        if (amount == 0) return true;
    }

    uint256 err;
    if (underlying == address(0)) {
        err = ctoken.mint{value: amount}(amount);
    }

Ref:

• Compound's cToken mint doc: https://compound.finance/docs/ctokens#mint
• Compound CEther.mint() https://github.com/compound-finance/compound-protocol/blob/v2.8.1/contracts/CEther.sol#L46
• Compound CErc20.mint() https://github.com/compound-finance/compound-protocol/blob/v2.8.1/contracts/CErc20.sol#L46

[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] Cache array length in for loops can save gas

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• CompoundHandler.sol#_getAccountBorrowsAndSupply()

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L135-L170

• CTokenRegistry.sol#_updateCTokenMapping()

  https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CTokenRegistry.sol#L61-L73

[S] Avoid unnecessary storage read can save gas

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/StakerVault.sol#L360-L389

    function unstakeFor(
        address src,
        address dst,
        uint256 amount
    ) public override returns (bool) {
        ILiquidityPool pool = controller.addressProvider().getPoolForToken(token);
        uint256 allowance_ = _allowances[src][msg.sender];
        require(
            src == msg.sender || allowance_ >= amount || address(pool) == msg.sender,
            Error.UNAUTHORIZED_ACCESS
        );
        require(balances[src] >= amount, Error.INSUFFICIENT_BALANCE);
        address lpGauge = currentAddresses[_LP_GAUGE];
        if (lpGauge != address(0)) {
            ILpGauge(lpGauge).userCheckpoint(src);
        }
        uint256 oldBal = IERC20(token).balanceOf(address(this));

        if (src != dst) {
            pool.handleLpTokenTransfer(src, dst, amount);
        }

        IERC20(token).safeTransfer(dst, amount);

        uint256 unstaked = oldBal - IERC20(token).balanceOf(address(this));

        if (src != msg.sender && allowance_ != type(uint256).max && address(pool) != msg.sender) {
            // update allowance
            _allowances[src][msg.sender] -= unstaked;
        }
        // ...
    }

L388 can use allowance_ - unstaked directly to avoid unnecessary storage read of _allowances[src][msg.sender] to save some gas.

Recommendation

Change to:

    function unstakeFor(
        address src,
        address dst,
        uint256 amount
    ) public override returns (bool) {
        ILiquidityPool pool = controller.addressProvider().getPoolForToken(token);
        uint256 allowance_ = _allowances[src][msg.sender];
        require(
            src == msg.sender || allowance_ >= amount || address(pool) == msg.sender,
            Error.UNAUTHORIZED_ACCESS
        );
        require(balances[src] >= amount, Error.INSUFFICIENT_BALANCE);
        address lpGauge = currentAddresses[_LP_GAUGE];
        if (lpGauge != address(0)) {
            ILpGauge(lpGauge).userCheckpoint(src);
        }
        uint256 oldBal = IERC20(token).balanceOf(address(this));

        if (src != dst) {
            pool.handleLpTokenTransfer(src, dst, amount);
        }

        IERC20(token).safeTransfer(dst, amount);

        uint256 unstaked = oldBal - IERC20(token).balanceOf(address(this));

        if (src != msg.sender && allowance_ != type(uint256).max && address(pool) != msg.sender) {
            // update allowance
            _allowances[src][msg.sender] = allowance_ - unstaked;
        }
        // ...
    }

[S] Using immutable variable can save gas

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/StakerVault.sol#L45

    address public token;

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/StakerVault.sol#L65-L67

    function initialize(address _token) external override initializer {
        token = _token;
    }

Considering that token will never change, changing it to immutable variable instead of storage variable can save gas.

[M] Setting uint variables to 0 is redundant

Setting uint variables to 0 is redundant as they default to 0.

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/pool/LiquidityPool.sol#L483

        uint256 currentFeeRatio = 0;

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L135

        for (uint256 i = 0; i < assets.length; i++)