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

Function `Staking::claimRewards` does not work as expected

carlosalbawork

Summary

Function Staking::claimRewards does not work as expected

Vulnerability Details

The function Staking::claimRewards is expected to give rewards to users that have staked tokens in the contract for weeks

​
function claimRewards() public {
uint256 soulmateId = soulmateContract.ownerToId(msg.sender);
// first claim
if (lastClaim[msg.sender] == 0) {
lastClaim[msg.sender] = soulmateContract.idToCreationTimestamp(
soulmateId
);
}
​
// How many weeks passed since the last claim.
// Thanks to round-down division, it will be the lower amount possible until a week has completly pass.
​
uint256 timeInWeeksSinceLastClaim = ((block.timestamp -
lastClaim[msg.sender]) / 1 weeks);
​
if (timeInWeeksSinceLastClaim < 1)
revert Staking__StakingPeriodTooShort();
​
lastClaim[msg.sender] = block.timestamp;
​
// Send the same amount of LoveToken as the week waited times the number of token staked
uint256 amountToClaim = userStakes[msg.sender] *
timeInWeeksSinceLastClaim;
loveToken.transferFrom(
address(stakingVault),
msg.sender,
amountToClaim
);
​
emit RewardsClaimed(msg.sender, amountToClaim);
}
​

However, it does not check when some tokens were deposited, only how much time passed since the user called Staking::claimRewards, so it is possible to claim rewards for tokens deposited seconds ago as if they were staked for weeks!

Impact

Breaks the whole purpose of the contract. There is no need to deposit the token in the contract weeks before claiming them.

Tools Used

Foundry

Proof of Concept:
1- User has LoveToken and has not called the function Staking::claimRewards in a long time (or never).
2- User deposits all the tokens he has
3- Just after, user calls Staking::claimRewards
4- The rewards are returned as if the tokens were staked there since the last time the user called Staking::claimRewards

Code
​
function test_ClaimRewardsWronglyRewarding() public {
uint balancePerSoulmates = 5 ether;
uint weekOfStaking = 5;
​
vm.prank(soulmate1);
soulmateContract.mintSoulmateToken();
​
vm.prank(soulmate2);
soulmateContract.mintSoulmateToken();
​
uint numberDays = balancePerSoulmates / 1e18;
vm.warp(block.timestamp + (numberDays * 1 days));
​
vm.prank(soulmate1);
airdropContract.claim();
​
vm.prank(soulmate2);
airdropContract.claim();
​
//prints 5 * 10 **18 ( balancePerSoulmates )
console2.log(loveToken.balanceOf(soulmate1));
​
//In the contract there are no tokens staked yet, but we advance 5 weeks
vm.warp(block.timestamp + weekOfStaking * 1 weeks + 1 seconds);
​
vm.startPrank(soulmate1);
​
loveToken.approve(address(stakingContract), balancePerSoulmates);
//We execute both calls one just after the other
stakingContract.deposit(balancePerSoulmates);
stakingContract.claimRewards();
vm.stopPrank();
​
//prints 5 * 5 * 10 **18 (weekOfStaking * balancePerSoulmates)
console2.log(loveToken.balanceOf(soulmate1));
​
//Therefore is not necessary to stake the tokens in the contract weeks for them to be claimed, just wait until enough time has passed, deposit and claim just after.
}
​

Recommendations

Recommended Mitigation:

Adding a call to Staking::claimRewards in Staking::deposit, therefore every time user deposits, it is given the rewards from the tokens staked since last time he deposited.

function deposit(uint256 amount) public {
if (loveToken.balanceOf(address(stakingVault)) == 0)
revert Staking__NoMoreRewards();
// No require needed because of overflow protection
+ claimRewards();
userStakes[msg.sender] += amount;
loveToken.transferFrom(msg.sender, address(this), amount);
emit Deposited(msg.sender, amount);
}
​

For that mitigation, Staking::claimRewards has to be slightly reworked as well.

function claimRewards() public {
uint256 soulmateId = soulmateContract.ownerToId(msg.sender);
// first claim
if (lastClaim[msg.sender] == 0) {
lastClaim[msg.sender] = soulmateContract.idToCreationTimestamp(
soulmateId
);
}
​
// How many weeks passed since the last claim.
// Thanks to round-down division, it will be the lower amount possible until a week has completly pass.
​
// q checkear que la fecha de 1 week aquí no falla como en le contrato de guardian vaults. También checkear performing division before multiplication can lead to precision loss. .SLITHER YELLOW
​
uint256 timeInWeeksSinceLastClaim = ((block.timestamp -
lastClaim[msg.sender]) / 1 weeks);
- if (timeInWeeksSinceLastClaim < 1)
- revert Staking__StakingPeriodTooShort();
+ if (timeInWeeksSinceLastClaim >= 1) {
lastClaim[msg.sender] = block.timestamp;
​
// Send the same amount of LoveToken as the week waited times the number of token staked
uint256 amountToClaim = userStakes[msg.sender] *
timeInWeeksSinceLastClaim;
loveToken.transferFrom(
address(stakingVault),
msg.sender,
amountToClaim
);
​
emit RewardsClaimed(msg.sender, amountToClaim);
+ }
}
​

Justification for the Mitigation given:

Maybe it is a bit difficult to understand why that mitigation would solve the problem. This example will serve as a justification and an explanation:

Case 1:

User1 minted the NFT, was given 3 tokens and deposited them 5 weeks ago, then deposited other 4 tokens 3 weeks ago and lastly 2 tokens just now

User1 never called the function Staking::claimRewards. Therefore the function sets the time passed as 5 weeks, when he minted the NFT. In the current version, then the function would return (3+4+2) * 5 = 45 tokens

Case 2:

User2 minted the NFT, was given 3 tokens 5 weeks ago, other 4 tokens 3 weeks ago and lastly 2 tokens just now. But he never deposited them in the contract. He just got them outside

User2 never called the function Staking::claimRewards. Therefore the function sets the time passed as 5 weeks, when he minted the NFT. So, he deposits his (3+4+2) tokens and calls the function just after. The result of the function Staking::claimRewards? (3+4+2) * 5 = 45 tokens

Wow, it did not matter that User1 staked some tokens 5 weeks ago! it only mattered that the function Staking::claimRewards was not called in that time! so both cases got same tokens in return, even though User2 staked all of them just seconds before the call to Staking::claimRewards!

In fact, not even Case 1 was managed properly, some tokens should have been weighted as being staked for 5 weeks, other for 3 and others for seconds. But all of them were taken into account as if the 9 tokens were staked all 5 weeks ago!

In case 1, Staking::claimRewards should return 35 (tokens * weeks staked) + 43 + 2*0 = 27
In case 2, Staking::claimRewards should return 9*0 = 0

What would happen if every time somebody deposited, it called Staking::claimRewards inside the Staking::deposit function before adding the tokens deposited to the userStakes:

Using Case 1:

1º: Deposits 3 tokens just after minting the nft -> calls Staking::claimRewards , first time after minting seconds ago, does not enter in the if clause because did not pass a week since minting, 0 rewards claimed

2: Deposits 4 tokens 2 weeks later -> calls Staking::claimRewards, 2 weeks since last call to Staking::claimRewards , 3*2 (tokens staked prior to current deposit * weeks since last call to Staking::claimRewards) = 6 rewards claimed in this call

3: Deposits 2 tokens 3 weeks later -> calls Staking::claimRewards, 3 weeks since last call to Staking::claimRewards , 7*3 (tokens staked prior to current deposit * weeks since last call to Staking::claimRewards) = 21 rewards claimed in this call

Total Rewards = 21 + 6 = 27 ! The amount that should be given in this case!

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.