code-423n4 / 2021-12-yetifinance-findings

0 stars 0 forks source link

`lastFeeTime` can be reset #201

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

cmichel

Vulnerability details

The ThreePieceWiseLinearPriceCurve.lastFeeTime variable is used in calculateDecayedFee to compute the elapsed time (decay = block.timestamp.sub(lastFeeTime)). This lastFeeTime variable is never initialized in the constructor but can repeatedly be set through setFeeCapAndTime. It's unclear why it should be allowed to reset the last fee time.

Impact

The whitelisted account can call setFeeCapAndTime to break the contract by setting it to a value in the future or set it in the past to get maximum decay.

Recommended Mitigation Steps

Consider initializing lastFeeTime in the constructor to the current time, setFeeCapAndTime should only set the lastFeePercent.

kingyetifinance commented 2 years ago

@LilYeti: This is a value entrusted to the multisig to be set, and when a new price curve is set it is set to 0. Since the logic in the price curve checks decay < decayTime, the decay will be huge since it is initialized to 0. So there is essentially no last fee on initialization which is good. And, when changing price curves, this is queried from the last contract before it is replaced so the last fee and percent stays the same.

alcueca commented 2 years ago

Downgrading to code clarity issue. This rationale would look good in natspec.