code-423n4 / 2022-09-frax-findings

2 stars 1 forks source link

No check if `rewardsCycleEnd` was there over a long time #270

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L45-L62

Vulnerability details

Impact

The user could buy shares with old price

Proof of Concept

if something happens and no one invokes syncRewards() for two or more rewardsCycle. Alice noticed that and decide to deposit() some frxEth with this old share price lat's say 1:5 but the real price if someone has invoked syncRewards() on the time the price will be 1:7 Now if someone invokes syncRewards() Alice will steal some rewards

Recommended Mitigation Steps

Add a period to rewardsCycleEnd lat’s say 3 days and check if block.timestamp >= rewardsCycleEnd + 3 days if no invoke syncRewards() to update the state

FortisFortuna commented 2 years ago

From @denett syncRewards should be called by us at the beginning of each period, or we need to automatically call it before deposits/withdrawals.

FortisFortuna commented 2 years ago

Duplicate of https://github.com/code-423n4/2022-09-frax-findings/issues/110