search

code-423n4 / 2021-09-yaxis-findings

0 stars 0 forks source link

`getMostPremium()` can be wrong #139

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xsanson

Vulnerability details

Impact

NativeStrategyCurve3Crv._harvest calls getMostPremium to get the best stablecoin to convert to. This function however is wrong in the case of balancesUSDC = balancesUSDT < balancesDAI, because it returns DAI, when it should be USDC or USDT. This is naturally a rare occasion, but a bad actor can set the balances (by depositing/withdrawing the Curve pool) like this just before the harvest function is called. Since this would imbalance even more the pool, the bad actor could also gain a profit by making the right swaps after the harvest.

Proof of Concept

https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/strategies/NativeStrategyCurve3Crv.sol#L83

Tools Used

editor

Recommended Mitigation Steps

Convert all < into <= inside getMostPremium().

BobbyYaxis commented 3 years ago

Harvest cannot be called by an external party to initiate, nor is there a warning when that is about to happen.

Haz077 commented 3 years ago

I totally agree, this should be labelled as low-risk

uN2RVw5q commented 3 years ago

I'll implement this.

I'll also do a refactoring along with this. There is no need to use a memory array, just regular local variables should be enough. It'll also be cheaper than going through memory.

uN2RVw5q commented 3 years ago

Implemented in https://github.com/code-423n4/2021-09-yaxis/pull/32

GalloDaSballo commented 2 years ago

Sponsor Mitigated Agree with low severity as funds are not at risk, and harvest cannot be just called by anyone Would recommend also using Flashbots or Private Transactions as further mitigation