search

code-423n4 / 2021-11-fei-findings

0 stars 0 forks source link

In TRIBERagequit.sol, users can get frontrunned #131

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

0x0x0x

Vulnerability details

Concept

When requery is called, token1OutBase can get updated. token1OutBase is a crucial parameter, which directly tells how many tokens will the user get.

If a user creates a transaction and before getting included in a block, token1OutBase gets reduced. Users will swap for an unexpected ratio and lose funds..

For swap operations in most contracts (like sushi), there is an input parameter for minimum outputted tokens. This ensures that the user can make swap requests with deterministic results. This protects users against such problems.

As seen in https://github.com/fei-protocol/fei-protocol-core/blob/cc14a601d60e6d550bc88ec84da0999fa8a3daf0/contracts/oracle/collateralization/CollateralizationOracleWrapper.sol#L128, minProtocolEquity can be set by governor or admin arbitrarily (indirectly also token1OutBase). On Oracle CachedValueUpdate event is emitted, but there is also no guarantee, if someone calls requery with new values or not. So users can easily get frontrunned or swap with an unexpected ratio.

Users don't have enough control when calling swap operation ngmi. There is also no protection for users against unexpected behaviour. I consider this a high-risk issue.

Mitigation steps

I strongly recommend adding events, view functions and a minimum output parameter for ngmi. Also, it would be nice to have a view function for token1OutBase and token0InBase. Minimum output parameter is utilized on almost all popular contracts with similar functionality, since it is crucial for user safety.

elee1766 commented 2 years ago

this is intended as far as i understand.

i see no reason to add a minimum output parameter as if minprotocolequity is set by admin/governor before the ragequit, the caller should lose their funds. that seems fine to me.

FEI may be burnt by the burner, so i see no real real protection added.

there is no need to add any events or view functions, the chain is available to be queried.

pauliax commented 2 years ago

As per the sponsor's comment, this is the intended behavior. Because the price is always adjusted down, users are incentivized to ngmi as fast as possible, and having an up-to-date minProtocolEquity sounds fair. However, I agree that slippage protection is a common practice to give users more control but in this case, it is not that severe, so I am assigning this issue a severity of low.