-
Notifications
You must be signed in to change notification settings - Fork 33
Description
This PR implemented tokenized gauges and was merged to prioritize audit availability.
There was concern on gauge system about a possible attack vector around flash loaning vault shares to boost in one block the balance of a user to get more rewards.
There was a discussion on that PR around having a decaying calculation or streaming of the rewards so that it cannot be gamed within one block.
The proposed solution on the PR was to add a kick function to update the boosted balance of a given account, but would be good to add more tests around validating if it entirely mitigates this scenario.
Issue found by yacademy:
The boosted balance for rewards is computed as balance * 0.4 + totalSupply * veYFIBalance / veYFITotalSypply * 0.6 . (Typo in veYFITotalSupply is copied from comment in L284 of Gauge.) Although depositing in your own account calls updateReward for your account, you can use another account to flash loan a large amount of vault tokens and deposit them to boost the totalSupply. Once you have boosted totalSupply, you trigger updateReward for the target account and then withdraw the flash loan deposit.
(Note that the inverse is also true. You can either sandwich attack someone else's update by withdrawing tokens before their update to reduce total supply, or you can withdraw tokens and use getRewardFor to force an update to someone else's rewards like discussed here: htt ps://github.com//issues/33.)
POC ( need checks against latest tokenized code updates to see validity):
Boosted Balance equation: https://github.com/YAcademy-Residents/veYFI/blob/7846291efec36638eae32ac8798544050ca20877/contracts/Gauge.sol#L302
Deposit Update Reward call: https://github.com/YAcademy-Residents/veYFI/blob/7846291efec36638eae32ac8798544050ca20877/contracts/Gauge.sol#L351
Total Supply increase: https://github.com/YAcademy-Residents/veYFI/blob/7846291efec36638eae32ac8798544050ca20877/contracts/Gauge.s ol#L362
There were a few suggestions against this possible attack from yacademy folk:
A possible solution is to compute a time weighted average of totalSupply. Alternatively, governance could set a veYFIBalance multiplier to use instead of using totalSupply/veYFITotalSupply. A third solution is to add require(block.timestamp > lastTimeRewardApplicable()); to the _updateReward() function to prevent a user from performing multiple deposits/withdrawals in the same block.