Curve AMO

[clement-ux]

Description

Create a new AMO for Base, on Curve.
Almost same AMO as current Convex AMO (on mainnet), with following exceptions:

• Depositing LPs on Curve gauge instead of Convex.
• Use WETH instead of raw ETH.
• Add _solvencyAssert() from AerodromeAMO logic.

Code Change Checklist

To be completed before internal review begins:

• The contract code is complete
• Executable deployment file
• Fork tests that test after the deployment file runs
• Unit tests *if needed
• The owner has done a full checklist review of the code + tests

Internal review:

• Two approvals by internal reviewers

[github-actions]

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against eda2e1e

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 153 lines in your changes missing coverage. Please review.

Project coverage is 49.81%. Comparing base (1817476) to head (e54d05a).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...acts/contracts/strategies/BaseCurveAMOStrategy.sol	0.00%	153 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2370      +/-   ##
==========================================
- Coverage   52.53%   49.81%   -2.73%     
==========================================
  Files          91       92       +1     
  Lines        4414     4567     +153     
  Branches     1162     1202      +40     
==========================================
- Hits         2319     2275      -44     
- Misses       2092     2289     +197     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[clement-ux]

Author code review

Requirements

What is the PR trying to do? Is this the right thing? Are there bugs in the requirements?

Easy Checks

Authentication

• Never use tx.origin
• Every external/public function is supposed to be externally accessible
• Every external/public function has the correct authentication

Ethereum

• Not handling raw ETH.

Cryptographic code

• No Cryptographic.

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.

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.

Overflow

• Code is solidity version >= 0.8.0
• All for loops use uint256

Proxy

• No storage variable initialized at definition when contract used as a proxy implementation.

Events

• All state changing functions emit events

Medium Checks

Rounding and casts

• Contract rounds in the protocols favor

• Contract does not have bugs from loosing rounding precision

• Code correctly multiplies before division

• Contract does not have bugs from zero or near zero amounts

• [] Safecast is aways used when casting

  • A lot of SafeCast are missing. However, this is from previous implementation that is already in use for long time. I will leave it like this to minimize contract modifications.

• Note about rounding:

  • calcTokenToBurn() rounding could lead to burning slightly less lpToken than required. However, this is not an issue because if there is not enough weth to transfer, tx will fail.
  • checkBalance() and _solvencyAssert() rounding could lead to a slightly underestimated value in the strategy. This is not an issue, this allows for a more cautious approach, it could be dangerous to over-estimate the value.

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
• If OpenZeppelin ACL roles are use review & enumerate all of them.
• Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

External calls

• Contract addresses passed in are validated
• No unsafe external calls
• Reentrancy guards on all state changing functions
  • [] Still doesn't protect against external contracts changing the state of the world if they are called.
• No malicious behaviors
• Low level call() must require success.
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Tests

• Each publicly callable method has a test
• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested
• If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing.

Deploy

• Deployer permissions are removed after deploy

Strategy Specific

Remove this section if the code being reviewed is not a strategy.

Strategy checks

• Check balance cannot be manipulated up AND down by an attacker
• No read only reentrancy on downstream protocols during checkBalance
• All reward tokens are collected
• The harvester can sell all reward tokens
• No funds are left in the contract that should not be as a result of depositing or withdrawing
• All funds can be recovered from the strategy by some combination of depositAll, withdraw, or withdrawAll()
• WithdrawAll() can always withdraw an amount equal to or larger than checkBalances report, even in spite of attacker manipulation.
• WithdrawAll() cannot be MEV'd
• Strategist cannot steal funds

Downstream

• We have monitoring on all backend protocol's governances
• We have monitoring on a pauses in all downstream systems

Thinking

Logic

Are there bugs in the logic?

• Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).

Deployment Considerations

Are there things that must be done on deploy, or in the wider ecosystem for this code to work. Are they done?

• No, nothing to be done on deploy.

Internal State

• What can be always said about relationships between stored state.
  • Outside from harvesterAddress and rewardTokenAddresses, there are no storage variable (outside of Initializable and Governor).
• What must hold true about state before a function can run correctly (preconditions)
  • Before deposit, vault should have send funds to strategy.
  • Before deposit and mintAndAddOTokens, strategy should have approved weth and oeth spending on curve pool and lpToken to the gauge.
• What must hold true about the return or any changes to state after a function has run.
  • After deposit/withdraw/mintAndAddOTokens/removeAndBurnOTokens/removeOnlyAssets the pool balance should have been improved, and protocol should remain solvent (_solvencyAssert()).
  • Only withdrawAll() doesn't have this _solvencyAssert() checks. This is done on purpose, to allow Vault or Governor to rescue funds in case of emergency.

Does this code do that?

• Yes it does.

Attack

What could the impacts of code failure in this code be.

1. Miscalculation of the totalValue held by this strategy (with checkBalance()).
2. Miscaclulation of calcTokenToBurn().

What conditions could cause this code to fail if they were not true.

1. Wrong value returned by get_virtual_price() on Curve Pool.
2. Wrong math.

Does this code successfully block all attacks.

1. Partially.
2. Yes.

Flavor

Could this code be simpler?
Maybe, but this isn't intended here.

Deployment of this contract need to be a copy from https://github.com/OriginProtocol/origin-dollar/blob/master/contracts/contracts/strategies/ConvexEthMetaStrategy.sol, with as little change as possible.

Could this code be less vulnerable to other code behaving weirdly?

[sparrowDom]

The strategy contract shares a lot of similarities with the ConvexEthMetaStrategy contract. I think we should refactor the common pieces of code into a separate class

[sparrowDom]

This abi is required here: https://github.com/OriginProtocol/origin-dollar/blob/master/brownie/world_base.py#L25

[sparrowDom]

this is really cool @DanielVF how you managed to incorporate the graph drawing bit into brownie scripts not requiring the Jupyter notebooks, and transferring data between the 2 environments.

[naddison36]

that's a nice way of grouping the config

[sparrowDom]

I like this one as well, using it in the future

[sparrowDom]

confirming the order is correct:

[sparrowDom]

Requirements

PR is creating an AMO strategy on Base using the OToken & WETH assets.

Easy Checks

Authentication

• Never use tx.origin
• [] Every external/public function is supposed to be externally accessible
  🟡 checkBalance should be external instead of public
• Every external/public function has the correct authentication

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.
• Contract is not vulnerable to being sent self destruct ETH

Cryptographic code

no own crypto

Gas problems

no loops

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.

Overflow

• Code is solidity version >= 0.8.0

Proxy

• No storage variable initialized at definition when contract used as a proxy implementation.
• 🟡 set the 0 governor to implementation contract (by calling _setGovernor(address(0)); in the constructor)

Events

• All state changing functions emit events

Medium Checks

Rounding and casts

• Contract rounds in the protocols favor
• Contract does not have bugs from loosing rounding precision
• Code correctly multiplies before division
• Contract does not have bugs from zero or near zero amounts
• Safecast is aways used when casting

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
• If OpenZeppelin ACL roles are use review & enumerate all of them.
• Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

External calls

• Contract addresses passed in are validated
• No unsafe external calls
• Reentrancy guards on all state changing functions
  • Still doesn't protect against external contracts changing the state of the world if they are called.
• No malicious behaviors
• Low level call() must require success.
• No slippage attacks (we need to validate expected tokens received)
  • 🟠 In the first Curve implementation we checked the pool code to confirm that the minAmount passed to add_liquidity would be respected. If we intend to use the same code for multiple AMOs we should manually check after add_liquidity call that expected amount of LP tokens has been received:

  require(lpDeposited >= minMintAmount, "Min LP amount error");
  

• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Tests

• Each publicly callable method has a test
• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested
• If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing.

Deploy

• Deployer permissions are removed after deploy

Strategy Specific

Strategy checks

• Check balance cannot be manipulated up AND down by an attacker
• No read only reentrancy on downstream protocols during checkBalance
• All reward tokens are collected
• The harvester can sell all reward tokens
• No funds are left in the contract that should not be as a result of depositing or withdrawing
• All funds can be recovered from the strategy by some combination of depositAll, withdraw, or withdrawAll()
• WithdrawAll() can always withdraw an amount equal to or larger than checkBalances report, even in spite of attacker manipulation.
• WithdrawAll() cannot be MEV'd
• Strategist cannot steal funds

Downstream

• We have monitoring on all backend protocol's governances
• We have monitoring on a pauses in all downstream systems

Thinking

Logic

Are there bugs in the logic?

• Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).

Deployment Considerations

nothing special needs to happen on deploy

Internal State

The code makes sure that the AMO position (and pool's self token ratio) in the pool after depositing always has at least 1:1 (OETH : WETH) ratio and in the worst case 2:1 (OETH : WETH) ratio.

Does this code do that?
Yes

Attack

If there were errors in slippage / expected token configurations we could be loosing user funds.

For example a faulty get_virtual_price on the curve pool could be problematic.

Flavor

Code is written quite nicely. We've tried to refactor bits of it, but it turned out to be worse. There might be room for improvement, to put some of the more math intensive bits of code into a library.