search

code-423n4 / 2021-06-gro-findings

0 stars 1 forks source link

Unauthorized rebalanceTrigger calls may allow one to exploit arbitrage opportunity and put system at risk #66

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

The need for an externally visible rebalanceTrigger() (when rebalance() does that check itself) is apparently that the whitelisted bot checks trigger before calling the very expensive/security-sensitive rebalance() operation which again checks to see if anything has changed between then and the previous trigger.

Exposing the rebalance trigger check externally for convenience may offer a front-running arbitrage opportunity to a non-whitelisted i.e. any bot which can check when a rebalance will be triggered by a whitelisted bot and then using that information to arbitrage on underlying stablecoins/strategies, which may affect system exposure.

Discussion with the project team reported that this is technically possible but only within the BP limit (25-50) of the current vs cached price (where the BASIS_POINTS is currently set to 20). If not, the Buoy safetyCheck will fail.

Proof of Concept

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/insurance/Insurance.sol#L187-L196

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/insurance/Insurance.sol#L198-L215

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/pools/oracle/Buoy3Pool.sol#L30

Tools Used

Manual Analysis

Recommended Mitigation Steps

Recommend adding onlyWhitelist modifier to rebalanceTrigger() which allows retaining the convenience of bots, but only whitelisted ones, checking before calling rebalance. This makes it only a little safer because one can always front-run the actual rebalance call. This will only force bots to monitor mempool for rebalances instead of arbing ahead of time. Revisit this aspect for any missed considerations.

kitty-the-kat commented 3 years ago

There is price check before rebalance. It is not very useful to add whitelist on view function. Because the code is public, anyone can implement a local function easily.

ghoul-sol commented 3 years ago

Solution proposed by warden does not solve the problem but the problem still remains valid. The issue looks quite generic and it's really a MEV problem that most protocols have. For that reason, I think it's reasonable to degrade to low risk.