DAO Reviews

Polygon Yield Peer Review

Hey fellow reviewers!

I’m submitting this peer review request for a Smart Contract that uses AAVE to earn yield on Polygon (currently 25% APR)

Bounty
1000 USDC, to be split based on contribution + a gift if the project takes off

Code

Goals
There are 2 main goals with this review:

  1. Is the smart contract secure?
  2. Can you help me write a better way to partially withdraw without wasting all the gas?

1 Definition of Secure
Can the use of withdraw, deposit, balanceOfWant and getTotalValue be used to steal other people funds or is it stable enough to ensure people can withdraw at least what they deposited (minus any negative interest)?

2 Better _divestFromAAVE
Is there a better way to divest from aave up to amount?
Can this be reliably done?

If you also have advice on how to better unit test, especially when working with other protocols such as AAVE, I’ll take it.

Thank you for your time!

Hey Alex! I’ve just done a quick review with the findings below, I’m happy to do another run through afterwards if you’d like.

1. Is the smart contract secure?

Funds can potentially be locked or caused to be liquidated by sending different aTokens to the contract

The functions deposited, owed, canBorrow and canRepay are used to calculate the amount of MATIC the contract has deposited/borrowed from Aave - and so how much it can safely deposit/withdraw. This is done by querying the total value of assets deposits/borrowed and then calculating the equivalent value number of MATIC.

The issue arises because it doesn’t check that these assets are MATIC tokens. This leads to a situation where a malicious party can deposit a different token into Aave of behalf of the contract, giving it (say) aUSDC and the contract will think that it has deposited more MATIC then it has.

In the case of canRepay the contract can be made to think it can withdraw more MATIC than it owns, resulting in reverts when repaying debt. In the case of canBorrow, the contract will use the borrowing power granted by it’s deposit of aUSDC to borrow MATIC. The contract then takes up a directional position on this pair of assets and should a large price change occur it may experience a liquidation.

There’s no incentive for someone to send aTokens to this contract however this issue can be easily fixed. I would recommend instead directly querying the contract’s balance of aMATIC and Debt Tokens - Developers. This also removes the need to convert everything to a value denominated in MATIC when using different tokens.

Contract borrows such that it is below MIN_HEALTH

In the canBorrow function we don’t borrow such that we stop at the health factor MIN_HEALTH so the contract will counter-intuitively fall below that number. In the snippet below if healthFactor = MIN_HEALTH+1, then the contract will use up 95% of the remaining headroom which will take the contract below the stated minimum.

  function canBorrow() public view returns (uint256) {
    (
      uint256 totalCollateralETH,
      uint256 totalDebtETH,
      uint256 availableBorrowsETH,
      uint256 currentLiquidationThreshold,
      uint256 ltv,
      uint256 healthFactor
    ) = ILendingPool(LENDING_POOL).getUserAccountData(address(this));

    // We borrow only if we are above MIN_HEALTH
    if(healthFactor > MIN_HEALTH) {
      // 95% of converted to want from Eth
      uint256 maxValue = PRECISION.mul(availableBorrowsETH).mul(95).div(100).div(PRECISION);

      // 18 decimals
      return PRECISION.mul(maxValue).mul(10**18).div(getRate()).div(PRECISION);
    }

    return 0;
  }

While not a security issue in and of itself, it being named as MIN_HEALTH may lead to it being relied upon such that it causes funds to be locked under some conditions. We can instead calculate the borrow amount which take the contract to the desired health factor by rearranging the below formula for amountInEth:

(totalCollateralInETH * LTV) / (totalDebtInETH + amountInEth) == MIN_HEALTH

2. Can you help me write a better way to partially withdraw without wasting all the gas?

To be able to safely withdraw amount of MATIC we need to make sure that we have (ignoring precision) amount / ltv of headroom of collateral to withdraw before we hit our limit on the health factor. Note this is the LTV of the asset, not the user so you’ll have to pull it from Aave separately, see LendingPool - Developers

We may need to do a few iterations of paying back loans to get to the point where we can withdraw this much MATIC, the pseudocode for when we can perform this final withdrawal is

function canWithdrawFunds (uint256 amount) internal returns (bool) {
    /* Pull data from Aave */
    if (totalDebtInETH == 0) return true;
    uint256 amountInETH = getEthValue(amount)
    return (totalCollateralInETH - amountInETH) * LTV / totalDebtInETH > MIN_HEALTH;
}

Relevant Aave code: protocol-v2/GenericLogic.sol at baeb455fad42d3160d571bd8d3a795948b72dd85 · aave/protocol-v2 · GitHub

Posting the relevant function here:

function _divestFromAAVE (uint256 amount) internal {
    require(amount <= getTotalValue(), "Cannot withdraw more than totalValue");
    
    // Loop to repay debt until we have enough headroom to withdraw safely
    while (!canWithdrawFunds(amount)) {
      _withdrawStepFromAAVE(canRepay());
    }

    // Withdraw the desired amount
    ILendingPool(LENDING_POOL).withdraw(want, amount, address(this));
}

This could be optimised further as the final iteration of _withdrawStepFromAAVE still repays the maximum amount of debt possible rather than just what’s needed to allow the withdrawal.

Misc notes

PRECISION does not increase precision of calculations

In quite a few cases we have a calculation which is in the format shown below

function fromSharesToWithdrawal(uint256 amount) public view returns (uint256){
    return PRECISION.mul(amount).mul(getTotalValue()).div(totalSupply()).div(PRECISION); 
  }

This doesn’t look to me to increase precision except in the case where you would have a poor order of operations in the central calculation (which I can’t see in the codebase).

Unit testing against Aave

For testing against Aave, I’d recommend just forking Kovan and running tests against that when it’s not possible to mock it locally. Hardhat allows forking quite easily:
[https://]hardhat[.]org/guides/mainnet-forking.html

1 Like

Thank you for the great advice Tom!
Will be implementing your suggestions in the next iteration.

If you don’t mind I’ll ask you to double check that I implemented them properly.

Kindly send me an address for the bounty!

Rewards paid here: 0x953fa5b380c934daf9a8cfa45af4926b29d7766dba901d557cd8f4e1b793b76c