TempleGold
TempleDAO
Foundry
25,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Users staking in TempleGoldStaking will have more than expected claimable rewards and create a DoS as for a user the rewardPerToken also contains the rewardPerToken from starting due to unupdated `userRewardPerTokenPaid` on staking

shikhar229169

Relevant Github Links

https://github.com/Cyfrin/2024-07-templegold/blob/main/protocol/contracts/templegold/TempleGoldStaking.sol#L495
https://github.com/Cyfrin/2024-07-templegold/blob/main/protocol/contracts/templegold/TempleGoldStaking.sol#L463

Summary

The Staking keeps a track of global rewardPerTokenStored and for a particular user the userRewardPerTokenPaid, which ensures that the user will get their reward subtracting their previously claimed reward.

When a user opens a stake in future within the current reward distribution period they will have their userRewardPerTokenPaid for that stake as 0 which was expected to be updated to rewardPerTokenStored so that they will be excluded from all the previous reward accumulated and have a start with current reward distribution, but due to this missing implementation the user will be included in the rewards distribution from the starting and have their claimable rewards more than expected and will make (Total Claimable Rewards of all Users > Total TGLD avaiable for claims for current distribution period) and thus creates a DoS for withdrawal and other stuffs.

Vulnerability Details

The vulnerability is present in the TempleGoldStaking::_applyStake function where it lags the implementation to update the userRewardPerTokenPaid which makes the users participating to have claimable rewards from the starting value of rewardPerTokenStored which will thus increase their claimable rewards by a large amount and creates a Denial of Service for claiming of rewards, as the total claimable rewards are more than TGLD claim avaiable for a particular distribution period, thus some users will not be able to claim.

When a user stakes, the rewardPerTokenStored is updated to calculate it for the current timestamp by including the previous supply of stakes before the user has staked so the user will be expected to receive their rewards from when they have deposited for an ongoing reward distribution period, but due to the missing updation of userRewardPerTokenPaid when a user stakes, it makes it calcuate their rewards by considering the previous rewards.

Impact

  • As the claimable rewards for users will be much larger than they actually should get, will make the total claimable rewards of user much more than the actual TGLD amount available for claim, thus some or all users will not be able to perform their claim.

  • If minted TGLD is supplied to TempleGoldStaking, so after the reward for a period is consumed then this TGLD which was for next distribution will be consumed and thus creating DoS for the whole Staking system.

PoC

Add the below coded PoC in the TempleGoldStakingTest contract present in file: test/forge/templegold/TempleGoldStaking.t.sol

Run the test:

forge test --mt test_ClaimableRewardsAreAllocated_MoreThanWhatWasExpected
function test_ClaimableRewardsAreAllocated_MoreThanWhatWasExpected() public {
// send temple gold to Temple Gold Staking
// to keep things simple, lets say the reward duration is of 2 weeks and vesting period is of 1 week
// let the tgld amount be such that reward rate is 1 tgld per second
vm.startPrank(executor);
staking.setVestingPeriod(1 weeks);
staking.setRewardDuration(2 weeks);
staking.setRewardDistributionCoolDown(0);
vm.stopPrank();
uint256 tgldTotalRewardAmount = 2 weeks * 1 ether;
deal(address(templeGold), address(staking), tgldTotalRewardAmount);
​
vm.prank(address(templeGold));
staking.notifyDistribution(tgldTotalRewardAmount);
​
​
deal(address(templeToken), address(this), 1000000 ether);
templeToken.approve(address(staking), type(uint256).max);
​
uint256 stakeAmt = 50 ether;
// stake for alice
staking.stakeFor(alice, stakeAmt);
​
// start the distribution reward
staking.distributeRewards();
​
// now after 1 week bob stakes
vm.warp(block.timestamp + 1 weeks);
staking.stakeFor(bob, stakeAmt);
​
// alice claims current reward
uint256 aliceLatestStakeIdx = staking.getAccountLastStakeIndex(alice);
staking.getReward(alice, aliceLatestStakeIdx);
​
// now after 1 week, bob vesting period will be over
vm.warp(block.timestamp + 1 weeks);
​
// now alice and bob again claims their reward
uint256 bobLatestStakeIdx = staking.getAccountLastStakeIndex(bob);
uint256 aliceCurrentClaimable = staking.earned(alice, aliceLatestStakeIdx);
uint256 bobCurrentEarnedClaimable = staking.earned(bob, bobLatestStakeIdx);
uint256 totalCurrentClaimable = aliceCurrentClaimable + bobCurrentEarnedClaimable;
​
// As the reward for bob was calculated considering the previous previous reward per token also
// therefore the totalCurrentClaimable will be more than current avaiable rewards
// and one of them will not be able to claim their reward
// Because bob was allocated more rewards than they were actually expected to get
​
assert(totalCurrentClaimable > templeGold.balanceOf(address(staking)));
​
// alice claims
// even if Alice doesn't make a claim, the bob claim will always revert because
// the claimable amt calculated is higher than the total amount available for claim
staking.getReward(alice, aliceLatestStakeIdx);
​
// bob claim reverts
vm.expectRevert();
staking.getReward(bob, bobLatestStakeIdx);
}

Tools Used

Manual Review, Unit Test in Foundry

Recommendations

When a user peforms a stake, then it should be ensured that they are not included in the previous rewards, therefore in the _applyStake function update the userRewardPerTokenPaid for that user's stake to current rewardPerTokenStored so that while calculating their reward they will only be included in their current period and not in previous reward period from the starting.

function _applyStake(address _for, uint256 _amount, uint256 _index) internal updateReward(_for, _index) {
totalSupply += _amount;
_balances[_for] += _amount;
_stakeInfos[_for][_index] = StakeInfo(uint64(block.timestamp), uint64(block.timestamp + vestingPeriod), _amount);
+ userRewardPerTokenPaid[_for][_index] = uint256(rewardData.rewardPerTokenStored);
emit Staked(_for, _amount);
}

Now after applying the recommendation, the test fails due to a failing assert which shows that now the rewards are allocated correctly.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Future stakers are paid with rewards that have been accrued from the past due to miscalculation in userRewardPerTokenPaid and _perTokenReward

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.