Aerodrome OETH AMO #2019
Aerodrome OETH AMO #2019
Conversation
* Add Tooling for Base network * Fix scripts * Add empty migrations file * Fix failing fork tests
PraneshASP
requested review from
sparrowDom,
DanielVF,
naddison36 and
shahthepro
as code owners
| * @param r2 The reserve of OETH. | ||
| * @return The calculated constant K. | ||
| */ | ||
| function getK(uint256 r1, uint256 r2) internal pure returns (uint256) { |
There was a problem hiding this comment.
Since this is an internal function, it should get an underscore.
|
|
||
| function _lpWithdraw(uint256 _lpTokensAmount) internal { | ||
| // Claim remaining rwards before withdrawing LP Tokens | ||
| aeroGaugeAddress.getReward(address(this)); |
There was a problem hiding this comment.
Do we lose these reward tokens if we don't withdraw them? If not, it might just make things easier to not withdraw them until they are ready to be passed on. Less gas, and less things that could go wrong during a withdraw.
| * the pool's balance is not worsened. | ||
| * Withdrawals are proportional so doesn't change the pools asset balance. | ||
| */ | ||
| modifier improvePoolBalance() { |
There was a problem hiding this comment.
This method should check the pool's balances before the action that the modifier wraps, then compare them again after.
This way we check that things got better during the action.
If I'm reading this correctly, this only compares the balances/ratios after the action has taken place, which means it will not be able to compare the before and after to see if the difference is better?
What is this modifier currently trying to do?
There was a problem hiding this comment.
What is this modifier currently trying to do?
This modifier is used to make sure after rebalancing, the pool ratio doesn't skew more than the allowed threshold. As long as the ratio remains within threshold, we should be good. It helps avoid errors from the strategist while rebalancing the pool. This also gives the strategist the flexibility to rebalance the pool as per the market conditions.
This way we check that things got better during the action.
To validate that we should define what we mean by "better" for the pool. Does it mean the pool should be closer to 50:50 ratio or should it be unbalanced till the safest point.
| * 10 digits. This way we move the decimal point by 36 places when doing the calculation | ||
| * and again by 36 places when we are done with it. | ||
| */ | ||
| uint256 k = (1e36 * IERC20(address(lpTokenAddress)).totalSupply()) / |
There was a problem hiding this comment.
Might want to avoid calling this 'k' because it's a very different thing than the AMM's k, which is also calculated elsewhere in the code.
| poolWETHBalance; | ||
| // prettier-ignore | ||
| // slither-disable-next-line divide-before-multiply | ||
| uint256 diff = (_wethAmount + 1) * k; |
There was a problem hiding this comment.
This calculation currently calculates a ratio, then uses that ratio to calculate the end result. This is slightly lossy in doing two calculations with rounding errors, which requires the extra high intermediate precision.
Would this be much simpler and less if we just move the (_wethAmount+1) to be directly multiplied by the totalSupply in the first step?
Something like:
(_wethAmount+1) * totalSupply / poolWETHBalance ?
| uint256 _amountIn, | ||
| uint256 _minAmountOut, | ||
| address _tokenIn, | ||
| address _recipient |
There was a problem hiding this comment.
The strategist should not be able to steal tokens from the strategy / vault. We should probably remove the _recipient option here.
There was a problem hiding this comment.
If we remove the _recipient, where should the swapped weth sent? Will it be held in the AMO contract?
If you made a contract change, make sure to complete the checklist below before merging it in master.
Refer to our documentation for more details about contract security best practices.
Contract change checklist:
Notion resources: