Open sherlock-admin3 opened 5 months ago
This issue has been marked as invalid for the following reasons:
In conclusion, there won't be any loss of funds and the contract can simply be redeployed.
@bjp333 What do you think about this issue?
I think this issue is a bit of a stretch. We will definitely have more than 1 wei staked prior to any distributions taking place.
Escalate
The overflow scenario does not only occur during the beginning of the lock contract. As stated in the original issue, the beginning of the contract is only given as an example, since it is where this is likely to happen.
This issue may also occur when the contract has been active for a while. The reward tokens are already set, and due to users staking/unstaking, if there exists a moment where the lockedSupplyWithMultiplier
is too little, this overflow issue will still occur.
Also, the original code itself handles the case where lockedSupplyWithMultiplier
is zero, so it only makes sense that it should also handle where lockedSupplyWithMultiplier
is 1 wei or 1e3 wei or something.
function _notifyReward(address _rewardToken, uint256 reward) internal {
> if (lockedSupplyWithMultiplier == 0)
> return; // If there is no locked supply with multiplier, exit without adding rewards (prevents division by zero).
Reward storage r = rewardData[_rewardToken]; // Accesses the reward structure for the specified token.
uint256 newReward = reward * 1e36 / lockedSupplyWithMultiplier; // Calculates the reward per token, scaled up for precision.
r.cumulatedReward += newReward; // Updates the cumulative reward for the token.
r.lastUpdateTime = block.timestamp; // Sets the last update time to now.
r.balance += reward; // Increments the balance of the token by the new reward amount.
}
Note that if this overflow issue happens, the entire protocol would brick. This is because reward calculation is used for all operations, including staking, early exiting, late exiting, and getting rewards. This means users would not be able to withdraw their staked tokens nor rewards once cumulatedReward
overflows, which is a disaster.
Quoting the definition of a medium severity issue, this issue would fall under "cause loss of fund" but "requires certain external conditions or specific states", and the loss itself definitely exceeds "small, finite amount of funds".
Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
Escalate
The overflow scenario does not only occur during the beginning of the lock contract. As stated in the original issue, the beginning of the contract is only given as an example, since it is where this is likely to happen.
This issue may also occur when the contract has been active for a while. The reward tokens are already set, and due to users staking/unstaking, if there exists a moment where the
lockedSupplyWithMultiplier
is too little, this overflow issue will still occur.Also, the original code itself handles the case where
lockedSupplyWithMultiplier
is zero, so it only makes sense that it should also handle wherelockedSupplyWithMultiplier
is 1 wei or 1e3 wei or something.function _notifyReward(address _rewardToken, uint256 reward) internal { > if (lockedSupplyWithMultiplier == 0) > return; // If there is no locked supply with multiplier, exit without adding rewards (prevents division by zero). Reward storage r = rewardData[_rewardToken]; // Accesses the reward structure for the specified token. uint256 newReward = reward * 1e36 / lockedSupplyWithMultiplier; // Calculates the reward per token, scaled up for precision. r.cumulatedReward += newReward; // Updates the cumulative reward for the token. r.lastUpdateTime = block.timestamp; // Sets the last update time to now. r.balance += reward; // Increments the balance of the token by the new reward amount. }
Note that if this overflow issue happens, the entire protocol would brick. This is because reward calculation is used for all operations, including staking, early exiting, late exiting, and getting rewards. This means users would not be able to withdraw their staked tokens nor rewards once
cumulatedReward
overflows, which is a disaster.Quoting the definition of a medium severity issue, this issue would fall under "cause loss of fund" but "requires certain external conditions or specific states", and the loss itself definitely exceeds "small, finite amount of funds".
Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Even though it is true that this issue can be triggered when the contract has been active for a while, it would require that the staked value is almost null, which is highly unlikely. Because the staking token is GAMMA, which is a token currently valued at ~0.14 USD and with 18 decimals, it's almost impossible that during the life of the contract, there are less than 1e18 tokens staked in total.
In a similar way, the classic vault inflation attack can always be triggered when totalSupply
is 0, but the only real risk is when a Vault is deployed because it's the only realistic scenario where the total value deposited is strictly 0.
I agree that this is unlikely to happen. However,
Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
Also, take into account that there will be stakes locked for 720 days, which belong to the team and investors, so the requirements necessary for this bug to appear are not realistic at all.
I don't believe it's fair to shield behind the expression "requires certain external conditions or specific states" to defend issues that will never be triggered in reality.
This issue has been marked as invalid for the following reasons:
- Before users start staking, the admins have to set the staking token.
- Before anyone can send rewards to the contract, the admins have to set the reward tokens.
- For the reasons above, the admins can set the staking token first, wait for some stakers, and later set the reward tokens and send some to the contract.
- Even in the improbable case that this attack is successfully executed, because the only staker is the attacker, the admins can simply redeploy a new contract and stake a minimum amount first to avoid this issue again. If some users have started staking between the attack and the admin's action, they can simply withdraw their locks.
In conclusion, there won't be any loss of funds and the contract can simply be redeployed.
I'd also like to dispute the initial reasons for invalidating this issue.
cumulatedReward
, he would have to be the first staker. He may even try to frontrun the original first staker. The point here is, after the attacker performs this attack, the staking of the following users would be affected. The admins can definitely just redeploy a new contract to fix this, but this would be unfair to users that has already staked in the original contract, and their tokens would be locked up for nothing.
- For point 3, there would be no incentive for stakers if the reward token is not set. It is very likely that only after the admins set the reward token, would the users begin to stake.
However, before users would begin to stake, the developers would have already configured the locks for the team and investors, making it impossible to trigger this issue because many tokens are already staked. Realistically, the only moment an attacker can trigger this bug is at market deployment, but it won't be possible to execute the attack if the developers configure the contract correctly and set the locks for the team and investors before setting any reward tokens.
Even under extreme circumstances where developers fail to configure the "team and investor" locks and deploy the contract, the worst outcome would necessitate redeploying the contract. If users have already staked in a compromised contract, admins could remove penalties, allowing users to withdraw their stakes early without consequences and reinvest in the new contract. Thus, even if this vulnerability were exploited, its actual impact would effectively be negligible.
This issue seems to be contigent on admin actions before the first notification of rewards. The difference between this and a first depositor inflation attack is the attack cannot immediately happen as long as the admin has taken the precautions (stake a minimum amount e.g. 1e18 before setting reward tokens), which is the case as well for ERC4626 vaults wherein a admin can deposit/mint a minimum amount of shares in deployment scripts
I also note #72 and #313 as possible duplicates. Will discuss internally with lead judge. Historically based on my judging experience, first inflation attack and its mitigations must be explicitly stated as a known risk, so I am inclined to believe this issue is valid for now, although still considering the extensive admin actions required highlighted by lead judge before notification of rewards is allowed
cc: @WangSecurity
Considering all the points @santipu03 said, I would say the likelihood is very low. Therefore, I believe this is informational at best.
I believe this issue to be valid with low likelihood and high impact because
Planning to accept escalation and make issue and duplicates a valid medium severity.
Result: Medium Has Duplicates
Considering issue #313 as the only duplicate of this issue asiIssue #72 fails to identify the correct attack path.
The protocol team fixed this issue in the following PRs/commits: https://github.com/GammaStrategies/StakingV2/pull/4
The Lead Senior Watson signed off on the fix.
pkqs90
medium
Integer overflow when calculating rewards
Summary
The protocol uses a Sushi Masterchef like method for maintaining rewards for each user. The
cumulatedReward
variable is used for keeping track of the accumulated amount of assets per staking token. However, sincecumulatedReward
is scaled by 1e36, it may lead to overflow during reward calculation.Vulnerability Detail
Let's see how the reward is calculated:
cumulatedReward
is maintained as the cumulative asset/share ratio scaled by 1e36.cumulatedReward
times the amount of shared token (with multiplier)The issue here is
cumulatedReward
is scaled by 1e36, which is too large, and is susceptible to grief attacks. An example is:notifyUnseenReward()
to accumulate the reward. ThecumulatedReward
is now 1e18 * 1e36 / 1 = 1e54.Note that uint256.max is around 1e78, and the limit would be easily hit if the tokens in step 1 and step 3 is 1000e18 instead of 1e18.
Impact
Integer overflow during reward calculation.
Code Snippet
Tool used
Manual review
Recommendation
Couple of ways for mitigation:
lockedSupplyWithMultiplier
can be initially set to 1e18 to avoidcumulatedReward
to be too large. The loss is be negligible for initial stakers.