Upgrade AAVE strategy to AAVE v2

[DanielVF]

Upgrade AAVE strategy to use AAVE v2.

Discussion for upgrade

We should plan on rolling this out as a new strategy address, just to guarantee a clean slate and that all addresses/state are correct. We'll remove the old strategy, then add this strategy.

Progress:

• Interface changes
• Mock changes
• Contract changes
• Tests pass
• Deployment written
• Deployment tested on fork with fork funds

Contract change checklist:

• Code reviewed by 2 reviewers. See template and example.
• Unit tests pass
• Slither tests pass with no warning
• Echidna tests pass if PR includes changes to OUSD contract (not automated, run manually on local)

[DanielVF]

Will write deploy script once we have the new deploy library running and merged. Contract changes ready for review.

[mikeshultz]

These changes LGTM.

[DanielVF]

Notes on AAVE Liquidity rewards:

• Official Liquidity Mining Docs
• Official Staking Docs
• rewards contract: 0xd784927Ff2f95ba542BfC824c8a8a98F3495f6b5
• stkAAVE: 0x4da27a545c0c5b758a6ba100e3a049001de870f5

1. Get rewards amount

rewards.getRewardsBalance(address[] calldata assets, address user)

2. Convert rewards into stkAAVE

rewards.claimRewards(address[] calldata assets, uint256 amount, address to)

3. Trigger cooldown

stkAAVE cannot be turned into AAVE without a five day cool down. Sending more funds in restarts the cool down, as does claiming rewards. Additionally, two days after the cool down unlocks, it locks again automatically. To restart the cool down clock:

stkAAVE.cooldown()

To find out your current cool down start time:

stkAAVE.stakersCooldowns(address(this))

4. Convert to AAVE

When the time is right:

stkAave.claimRewards(address(this), type(uint256).max) 

stkAave.redeem(address(this), stkAaveBalance)

All together

Because moving funds or claiming rewards resets the cool down, the order matters a little bit

1. First we convert stkAave.redeem to AAVE
2. Then we claim rewards from stkAave
3. Transfer AAVE to the vault to be sold
4. start the cooldown for the next round.

[franckc]

LGTM

[DanielVF]

Testing out AAVE Staking

stkAave = Contract.from_explorer("0x4da27a545c0c5b758a6ba100e3a049001de870f5")
aave = Contract.from_explorer("0x7fc66500c84a76ad7e9c93437bfc5ac33e2ddae9")
incentivesController = Contract.from_explorer("0xd784927Ff2f95ba542BfC824c8a8a98F3495f6b5")
VAULT = "0xE75D77B1865Ae93c7eaa3040B038D7aA7BC02F70"
ADAI_V2 = '0x028171bca77440897b824ca71d1c56cac55b68a3'
ME = "0x4a49985b14bd0ce42c25efde5d8c379a48ab02f3"
FROM_ME = {'from': ME}


stkAave.cooldown(FROM_ME)
chain.sleep(stkAave.COOLDOWN_SECONDS()+100)
chain.mine()

cooldown = stkAave.stakersCooldowns(ME)
windowStart = cooldown + stkAave.COOLDOWN_SECONDS()
windowEnd = windowStart + stkAave.UNSTAKE_WINDOW()
currentTimestamp = chain.time()
currentTimestamp > windowStart and currentTimestamp < windowEnd

stkAaveBalance = stkAave.balanceOf(ME)
rewardLiquidationThreshold = 50
stkAaveBalance > rewardLiquidationThreshold

stkAave.redeem(ME, stkAaveBalance, FROM_ME)
stkAave.balanceOf(ME)

aaveBalance = aave.balanceOf(ME)
aave.transfer(VAULT, aaveBalance, FROM_ME)

pendingRewards = incentivesController.getRewardsBalance([ADAI_V2], ME)
incentivesController.claimRewards([ADAI_V2], pendingRewards, ME, FROM_ME)

[DanielVF]

Now we have tests, next up is deploy scripts.

[franckc]

Reviewed the deploy script - minor comment inline otherwise lgtm.
I haven't reviewed the contract piece yet.

[mikeshultz]

Backward comparison? Wouldn't actual > _amount mean we withdrew more than expected?

Looking at the LendingPool source, actual is likely to only be reduced to the user's balance if the max int is provided as an argument for amount. Not that we should depend on this never changing since it's upgradable.

[DanielVF]

I guess this was just a cheap sanity check. Do you think it would be better to do actual == amount?

[mikeshultz]

IMO, I think that would be more appropriate since actual should never be greater than _amount. I was also suggesting the revert message wasn't accurate.

[DanielVF]

@tomlinton, could you take a look at the contract side of this when you have a chance?

[tomlinton]

Not sure if I previously suggested this, but any thoughts on maintaining the v1 strategy as is (in the codebase) and adding a new file for the v2 strategy stuff? Might be more pain than it is worth.

[tomlinton]

I think the reason I suggested the v1 vs v2 thing is that when I review the changes here I've got the blinders on with regard to how the rest of the strategy works because the stuff we originally had doesn't show up in the diff, e.g. checkBalance. Fortunately it's all reasonably simple.

[DanielVF]

Not sure if I previously suggested this, but any thoughts on maintaining the v1 strategy as is (in the codebase) and adding a new file for the v2 strategy stuff? Might be more pain than it is worth.

Yeah, maybe we talked in Discord. I'd rather keep the codebase simple, and we can all ways get the old strategy back from git in the unlikely event that we need it.

[tomlinton]

👍

[DanielVF]

Requirements

This PR moves from AAVE v1 to AAVE v2, and adds support for the AAVE v2 rewards.

Sheesh StkAAVE is a pain.

Deployment Considerations

AAVE v2 rewards collection requires that the collectRewardToken() method on the strategy be called by the vault on the correct times to hit the unstaking window. We may need a keeper contract to do this.

Internal State

No internal state is modified during operation. Contract storage slots are configuration. All dynamic state is looked up from external sources.

💡 If we were using a more recent solidity version, we could immutable all these configuration addresses into the contract, and save gas on everything the contract does.

Attack

AAVE very difficult to misuse.

Funds are only ever transferred out to the vault, or to the address specified by the vault.

Balances are a simple 1-1 mapping of aToken balances.

Logic

💡 We aren't handling collecting the small amounts of StkAAVE interest that could be generated from the week worth of StkAAVE we'll build up between converting it to AAVE. If AAVE reintroduces rewards mining, we can upgrade to support it.

Tests

• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested

Flavor

Could reorder methods for more standardization. Will do later though, if we do.

Overflow

• Never use "+" or "-", always use safe math
• Check that all for loops use uint256

Black magic

• Does not contain selfdestruct

• Does not use delegatecall outside of proxying

• (If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.)

• Address.isContract should be treated as if could return anything at any time, because that's reality.

Authentication

• Never use tx.origin
• Check that every external/public function should actually be external
• Check that every external/public function has the correct authentication

Cryptographic code

No crypto code used.

Gas problems

• Contracts with for loops must have either:
  • A way to remove items
  • Can be upgraded to get unstuck
  • Size can only controlled by admins
• Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.

External calls

• Contract addresses passed in are validated
• Unsafe external calls. AAVE and trusted stables only for external calls
• Reentrancy guards on all state changing functions
• Malicious behaviors. If AAVE is malicious, problems ensue.
• Could fail from stack depth problems (low level calls much require success)
• No slippage attacks (we need to validate expected tokens received)
• Oracles? none
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Don't call balanceOf for external contracts to determine what they will do, when they instead use internal accounting?

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.
• Contract does not use msg.value