Emergency withdrawal boolean from lp to underlyings

[petrovska-petro]

Tackles #27. Waiting on #25 on getting + approvals and merging to have here cleaner reviewing scope ideally

Considerations in _withdrawLpToUnderlyings method

• Opted for not keeping in storage poolId or pool addresses given that this method is available for special occasions to mitigate risks, gas savings should be minimal for an action 1 off-time
• PoolId/pool address are retrieved instead from the bpt itself or metaregistry in curve
• Curve lps may hold 2 or 3 tokens within them, method to withdraw represents a different signature. Leverage the number of coins to decide which one to use
• In Aura Avatar on the exit of the pool, the underlyings are directly sent to the owner as vault design allows the action gracefully
• In the case of Curve given that many pools presents different abi structure. While checking not all of them has the ability to send to _recipient directly while remove_liquidity. Have not found a reliable way on-chain so far to determine when it is or not possible. Opted for directly sending the underlying tokens using safeTransfer to the owner
• It is acceptable by default minAmountsOut as zero for this emergency method

[codecov[bot]]

Codecov Report

Merging #29 (ed21c35) into main (c245192) will decrease coverage by 0.55%. The diff coverage is 86.84%.

:exclamation: Current head ed21c35 differs from pull request most recent head dbdb8a3. Consider uploading reports for the commit dbdb8a3 to get more accurate results

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
- Coverage   95.85%   95.30%   -0.55%     
==========================================
  Files           8        8              
  Lines         651      682      +31     
  Branches       94      100       +6     
==========================================
+ Hits          624      650      +26     
  Misses          9        9              
- Partials       18       23       +5     

Impacted Files	Coverage Δ
src/convex/ConvexAvatarMultiToken.sol	96.28% <83.33%> (-1.31%)	:arrow_down:
src/aura/AuraAvatarMultiToken.sol	98.68% <92.85%> (-0.40%)	:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[GalloDaSballo]

Additional Notes

The Manager can rekt the Owner (Privilege Escalation)

-> Prepare Pool (by making it imbalanced so that Avatar rekt itself) -> Call _withdraw with emergency set to true -> Avatar takes IL / Rekt itself -> Manager rebalances the pool

Because I believe the only account that can make the decision is Owner, I recommend reverting back to sweeping to Owner and then Owner can withdraw (with a slippage check)

[gosuto-inzasheru]

@dapp-whisperer can you share your thoughts here, the feature was requested by you. is withdrawing to underlying indeed a market sell, and does it carry the risks mentioned? should the lower threshold msig have that kind of power?

[dapp-whisperer]

It is acceptable by default minAmountsOut as zero for this emergency method

I think the main issue is this, the owner should be able to place slippage limits. We should also independently confirm we get out the expected balances at the end of the function before sending to owner.

The other issue is the scope bloat, which I still believe is worthwhile for the ability to get out of positions quickly. We've proven to take a day+ to get consensus and get out of positions in emergency scenarios.

However, we have to be able to do it safely. If we have outstanding security concerns, which we do, we should disable this feature for now in any live deployments (hopefully that's a simple code change).

My recommendation is to sweep the LP token to Owner like before, and then Owner can deal with it via a call which is fully customized

This sounds good for now, is this still possible without the conversion to underlying?

If we have a smaller position that's a bit sus, we can keep it in treasuryOps to simulate the same permissions.

[petrovska-petro]

It is acceptable by default minAmountsOut as zero for this emergency method

I think the main issue is this, the owner should be able to place slippage limits. We should also independently confirm we get out the expected balances at the end of the function before sending to owner.

The other issue is the scope bloat, which I still believe is worthwhile for the ability to get out of positions quickly. We've proven to take a day+ to get consensus and get out of positions in emergency scenarios.

However, we have to be able to do it safely. If we have outstanding security concerns, which we do, we should disable this feature for now in any live deployments (hopefully that's a simple code change).

My recommendation is to sweep the LP token to Owner like before, and then Owner can deal with it via a call which is fully customized

This sounds good for now, is this still possible without the conversion to underlying?

If we have a smaller position that's a bit sus, we can keep it in treasuryOps to simulate the same permissions.

Slippage limits have couple of nuances to do it precisely on-chain, specially for example, Balancer post-PoS decided to not longer recommend TWAP oracles, which have being deprecated at their end. In the other hand, precise on-chain pricing its a whole project by itself.

Given that there is not a simple way to handle safely the slippage controls on-chain and there are standing concerns around the feature itself, it may be worthier to simply close it PR, and proceed with creating the emergency scripts directly from the owner, so at least if ever the circumstance arises, we can act as promptly as we manage

[petrovska-petro]

closing this pr given the security concern rase. sending c245192851bd4cdd2dd23b81390db0c43a31111e for security review before heading to test- runs