Sublime contest - leastwood's results

Handle

leastwood

Vulnerability details

Impact

The AaveYield contract provides users with the option to choose Aave as their strategy of choice to generate yield. Users will make deposits to this strategy via the SavingsAccount contract. Upon deposit, shares are minted at a 1:1 exchange rate and the user receives an equivalent amount of aTokens. As the strategy accrues yield, each user's shares increase in value in proportion to their claim on the underlying pool balance.

However, as shares are minted 1:1, any user who enters the pool after the accrual of yield, will therefore also have claim over the underlying rewards. Hence, malicious users could extract any yield by constantly entering and exiting the pool whenever the strategy is able to claim rewards.

Proof of Concept

Consider the following example:

• Alice enters the AaveYield contract by depositing 1000 tokens and in turn receives 1000 aTokens and 1000 shares at a 1:1 exchange rate. This is outlined in AaveYield._depositERC20.
• Over time, the Aave strategy earns 200 tokens such that withdrawing Alice's 1000 aTokens will return 1200 tokens to Alice.
• Bob sees Alice has earned yield but is yet to exit the pool. Hence, Bob decides to also enter the yield strategy by depositing 1000 tokens and receiving 1000 shares as well.
• Hence, the pool now has 2000 total shares and 2200 underlying tokens.
• Bob quickly withdraws their tokens from the strategy and receives 1100 tokens in return, despite having not spent any time in the pool.

As a result, Bob is able to extract yield from the pool without having to deposit their funds. By repeating this exploit, Bob can continue to extract yield generated by the protocol. Hence, this issue severely impacts how rewards are accrued for the protocol.

function getSharesForTokens(uint256 amount, address asset) external view override returns (uint256 shares) {
    shares = (amount.mul(1e18)).div(getTokensForShares(1e18, asset));
}

function _depositERC20(address asset, uint256 amount) internal returns (address aToken, uint256 sharesReceived) {
    aToken = liquidityToken(asset);
    uint256 aTokensBefore = IERC20(aToken).balanceOf(address(this));

    address lendingPool = ILendingPoolAddressesProvider(lendingPoolAddressesProvider).getLendingPool();

    //approve collateral to vault
    IERC20(asset).approve(lendingPool, 0);
    IERC20(asset).approve(lendingPool, amount);

    //lock collateral in vault
    AaveLendingPool(lendingPool).deposit(asset, amount, address(this), referralCode);

    sharesReceived = IERC20(aToken).balanceOf(address(this)).sub(aTokensBefore);
}

https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L130-L143 https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/yield/AaveYield.sol#L192-L209 https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/yield/AaveYield.sol#L290-L304

Tools Used

Manual code review Discussions with Ritik This issue was submitted late as I was still confirming some details with the sponsor after the contest had closed :/

Recommended Mitigation Steps

The Sublime team has been considering implementing a wrapper for aTokens such that tokens are not minted 1:1 but are instead in line with how the Yearn and Compound strategies operate. This should ensure that users who enter the strategy at a later date, mint an equivalent amount of shares equivalent to the exchange rate of 1:indexed interest.

Handle

leastwood

Vulnerability details

Impact

If for whatever reason the Chainlink oracle returns a malformed price due to oracle manipulation or a malfunctioned price, the result will be passed onto users, causing unintended consequences as a result.

In the same time it's possible to construct mitigation mechanics for such cases, so user economics be affected by sustainable price movements only. As price outrages provide a substantial attack surface for the project it's worth adding some complexity to the implementation.

Proof of Concept

https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/PriceOracle.sol#L149-L161

function getLatestPrice(address num, address den) external view override returns (uint256, uint256) {
    uint256 _price;
    uint256 _decimals;
    (_price, _decimals) = getChainlinkLatestPrice(num, den);
    if (_decimals != 0) {
        return (_price, _decimals);
    }
    (_price, _decimals) = getUniswapLatestPrice(num, den);
    if (_decimals != 0) {
        return (_price, _decimals);
    }
    revert("PriceOracle::getLatestPrice - Price Feed doesn't exist");
}

The above code outlines how prices are utilised regardless of their actual value (assuming it is always a non-zero value).

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider querying both the Chainlink oracle and Uniswap pool for latest prices, ensuring that these two values are within some upper/lower bounds of each other. It may also be useful to track historic values and ensure that there are no sharp changes in price. However, the first option provides a level of simplicity as UniswapV3's TWAP implementation is incredibly resistant to flash loan attacks. Hence, the main issue to address is a malfunctioning Chainlink oracle.

Handle

leastwood

Vulnerability details

Impact

The emergencyWithdraw function is implemented in all yield sources to allow the onlyOwner role to drain the contract's balance in case of emergency. The contract considers ETH as a zero address asset. However, there is a call made on _asset which will revert if it is the zero address. As a result, ETH tokens can never be withdrawn from the NoYield contract in the event of an emergency.

Proof of Concept

Consider the case where _asset == address(0). An external call is made to check the contract's token balance for the target _asset. However, this call will revert as _asset is the zero address. As a result, the onlyOwner role will never be able to withdraw ETH tokens during an emergency.

function emergencyWithdraw(address _asset, address payable _wallet) external onlyOwner returns (uint256 received) {
    require(_wallet != address(0), 'cant burn');
    uint256 amount = IERC20(_asset).balanceOf(address(this));
    IERC20(_asset).safeTransfer(_wallet, received);
    received = amount;
}

Affected function as per below: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/yield/NoYield.sol#L78-L83

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider handling the case where _asset is the zero address, i.e. the asset to be withdrawn under emergency is the ETH token.