First Flight #9: Soulmate
First Flight #9
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Incorrect staking rewards calculation allows anyone to obtain more tokens

0xlrivo

Summary

Staking::claimRewards() fails to correctly compute the amount of staking rewards a user should get.

Vulnerability Details

The vulnerability can be identified in the fact that the contract only tracks the last claim time (lastClaim) and computes the rewards based on the current staking balance (userStakes).
Anyone can call Staking::deposit() just before claming the rewards so that their userStakes is higher, meaning that they will receive more rewards.

Impact

Anyone can claim more tokens than normal by depositing before withdrawing.

Tools Used

Add the following unit test to StakingTest.t.sol:

function test_incorrectStakingRewardsCalculations() public {
uint256 tokensToDeposit = 100e18;
uint256 weeksOfStaking = 10;
uint256 expectedRewards = tokensToDeposit * 10;
​
loveToken.testMint(soulmate1, tokensToDeposit); // soulmate1 starts with 100 tokens
​
vm.startPrank(soulmate1);
loveToken.approve(address(stakingContract), type(uint256).max);
stakingContract.deposit(tokensToDeposit);
​
// fast forward of 10 weeks
vm.warp(block.timestamp + weeksOfStaking * 1 weeks + 1 seconds);
​
// just before claiming, he deposits 100 tokens more
loveToken.testMint(soulmate1, tokensToDeposit);
stakingContract.deposit(tokensToDeposit);
stakingContract.claimRewards(); // here userStakes[msg.sender] = 200
vm.stopPrank();
​
uint256 actualRewards = loveToken.balanceOf(soulmate1);
assert(actualRewards > expectedRewards);
assert(expectedRewards == 1000e18);
assert(actualRewards == 2000e18);
}

In the example above the user initially deposits 100 tokens. Normally, after 10 weeks, he sould be able to claim 1000 but, before claiming, he deposits 100 token more. This causes Staking::claimRewards() to calculate the rewards on 200 tokens of initial deposit (even if only half of them actually stayed in the contract for 10 weeks) and he's able to claim 2000 tokens.

Recommendations

Add another mapping that tracks the amount of claimable rewards for each user and use this mapping to correctly calculate the amount of staking rewards.

With the following precautions:

  • Soulmate::ownerToId() should revert if 0 is returned because we want to avoid non-minters to be able to use the staking contract functions

  • Staking::deposit() should update both the amount of claimable rewards and lastClaim BEFORE updating the userStakes mapping

  • Staking::deposit() should update both the amount of claimable rewards and lastClaim BEFORE updating the userStakes mapping

Example implementation
error Staking_notElgible();
​
mapping(address user => uint256 loveToken) public userStakes;
mapping(address user => uint256 claimable) public claimableRewards;
mapping(address user => uint256 timestamp) public lastDeposit;
​
function deposit(uint256 amount) public {
if (loveToken.balanceOf(address(stakingVault)) == 0)
revert Staking__NoMoreRewards();
// this will revert if 0, so it checks if this user has an NFT
require(soulmateContract.ownerToId(msg.sender) > 0);
​
if (lastDeposit[msg.sender] > 0) {
uint256 timeInWeeksSinceLastDeposit = ((block.timestamp - lastDeposit[msg.sender]) / 1 weeks);
claimableRewards[msg.sender] += userStakes[msg.sender] * timeInWeeksSinceLastDeposit;
}
userStakes[msg.sender] += amount;
lastDeposit[msg.sender] = block.timestamp;
loveToken.transferFrom(msg.sender, address(this), amount);
​
emit Deposited(msg.sender, amount);
}
​
function withdraw(uint256 amount) public {
require(userStakes[msg.sender] > 0);
uint256 timeInWeeksSinceLastDeposit = ((block.timestamp - lastDeposit[msg.sender]) / 1 weeks);
claimableRewards[msg.sender] += userStakes[msg.sender] * timeInWeeksSinceLastDeposit;
userStakes[msg.sender] -= amount;
loveToken.transfer(msg.sender, amount);
emit Withdrew(msg.sender, amount);
}
​
function claimRewards() public {
if (lastDeposit[msg.sender] == 0)
revert Staking_notElgible();
uint256 timeInWeeksSinceLastDeposit = ((block.timestamp - lastDeposit[msg.sender]) / 1 weeks);
uint256 oldClaimableRewards = claimableRewards[msg.sender];
if (timeInWeeksSinceLastDeposit < 1 && oldClaimableRewards == 0)
revert Staking__StakingPeriodTooShort();
​
uint256 amountToClaim = claimableRewards[msg.sender] + ( userStakes[msg.sender] * timeInWeeksSinceLastDeposit);
​
claimableRewards[msg.sender] = 0; // update the claimable rewards
lastDeposit[msg.sender] = block.timestamp;
​
loveToken.transferFrom(
address(stakingVault),
msg.sender,
amountToClaim
);
​
emit RewardsClaimed(msg.sender, amountToClaim);
}
Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-claimRewards-multi-deposits-time

High severity, this allows users to claim additional rewards without committing to intended weekly staking period via multi-deposit/deposit right before claiming rewards.

Support

FAQs

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