-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TOB-UNIFEE-1: notifyRewardAmount() can be called without transferring tokens #59
Comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Severity: Medium
Difficulty: High
Type: Data Validation
Found by: Echidna
Finding ID: TOB-UNIFEE-1
Target: UniStaker.sol#L574-L576
Description
notifyRewardAmount()
can be called without transferring the reward tokens first. This would result in insufficient balance by the contract to cover all reward claims.The impact of this is that users can claim more rewards than they are entitled to due to the rewardRate being artificially high. Eventually, there won't be enough reward tokens in the contract for all beneficiaries to withdraw, and the
claimRewards
transactions will revert. When this artificially inflated rewards amount gets high enough, the rewards feature is effectively disabled.This issues is made possible by a bug in the check that is meant to ensure the reward amount was transferred in with the call:
https://github.com/uniswapfoundation/scopelift/blob/d745dd2a393f4b6a35bca2fd72f4cd198840c081/src/UniStaker.sol#L574-L576
As shown, the check does not consider any unclaimed reward amounts. When the amount of unclaimed rewards is greater than the new total reward amount, then this check will always pass.
According to the development team, this function is only intended to be called from the claimFees() function in V3FactoryOwner and within that function, the transfer is made prior to this call and no other account will be set as the
rewardNotifier
. In this case, if the protocol is deployed as intended, there would be no way to even callnotifyRewardAmount()
directly if the onlyrewardNotifier
is the V3FactoryOwner.However, there is no guarantee the contracts will be deployed in this manner or that no other account will be set as a
rewardNotifier
. While the likelihood of this issue being exploited is extremely low, the consequences if occurred could be significant.Scenario
See PoC below showing an example of a staker, Eve, who receives double her fair reward due to this.
Mitigation
Short term, there is no clear cut mitigation for this. Here are some options:
Either way, it is important to document this issue in both the UniStaker and V3FactoryOwner contracts as well as any related external documentation.
Long term, stateful invariant tests can help identify issues such as this.
Proof Of Concept
Echidna
This issue was also identified by Echidna during our invariant testing campaign with the following call sequence:
Using this invariant check and associated handler:
The text was updated successfully, but these errors were encountered: