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

`Staking::claimRewards` calculates initial `lastClaim` of a user based on their soulmate nft creation timestamp instead of staking timestamp leading to incorrect staking rewards payout

shikhar229169

Summary

The Staking::claimRewards function is designed to allow users to claim rewards based on the number of weeks they have staked their tokens. However, there is a logical error in the initial setting of the lastClaim variable, which uses the creation timestamp of the user's soulmate NFT instead of the actual staking timestamp.

As a result of which it considers the staked time of LoveToken inside Staking contract on the basis of the time of the soulmate NFT creation timestamp and thus leads to incorrect calculation of rewards leading to extra payout.

Vulnerability Details

The vulnerability lies in the initialization of the lastClaim variable inside the Staking::claimRewards from line 73.

When a user claims rewards for the first time, the lastClaim timestamp is incorrectly set to the creation timestamp of the user's soulmate NFT (soulmateContract.idToCreationTimestamp(soulmateId)). This timestamp represents when the NFT was minted, not when the user began staking their tokens. As a consequence, the calculation of the number of weeks since the last claim (timeInWeeksSinceLastClaim) will be incorrect, leading to an erroneous reward amount.

Considering the NFT creation time as the timestamp for staked amount is irrelevant. First of all staking doesn't depend on soulmate NFT creation timestamp and along with that staking doesn't depend whether a user has a soulmate NFT or not and thus is not implemented correctly.

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);
}

Impact

It affects the normal functioning of the Staking contract as users will received extra rewards for their staked amount because the time of staking is incorrectly calculated from the soulmate NFT creation timestamp instead of actual staking.

PoC

Add the test in the file: test/unit/StakingTest.t.sol

Run the test:

forge test --mt test_ClaimRewards_ConsiderLastClaimFromNftCreation_LeadingToExtraRewards -vv
function test_ClaimRewards_ConsiderLastClaimFromNftCreation_LeadingToExtraRewards() public {
// ---------------- Setting Up For Test to claim LoveToken ❤️ -------------------------
vm.prank(soulmate1);
soulmateContract.mintSoulmateToken(); // 💖
​
uint256 relationshipCreationTime = block.timestamp;
​
vm.prank(soulmate2);
soulmateContract.mintSoulmateToken(); // ❤️
​
vm.warp(block.timestamp + 1 days);
​
// soulmate1 collects their airdrop for 1 day of being into relation
vm.prank(soulmate1);
airdropContract.claim();
​
// soulmate1 now has 1 LoveToken ❤️
assertEq(loveToken.balanceOf(soulmate1), 1e18);
​
// -------------------------- Actual Testing For Staking Begins ---------------------------
​
// 123 days later
vm.warp(block.timestamp + 123 days);
​
// soulmate1 deposits 1 LoveToken ❤️
vm.startPrank(soulmate1);
loveToken.approve(address(stakingContract), 1e18);
stakingContract.deposit(1e18);
vm.stopPrank();
​
// 1 week later
vm.warp(block.timestamp + 1 weeks);
​
uint256 balanceBeforeClaim = loveToken.balanceOf(soulmate1);
​
// claim the rewards
vm.prank(soulmate1);
stakingContract.claimRewards();
​
// now according to the rule, for 1 token deposited for 1 week, 1 LoveToken should be rewarded
// but protocol considers the staking time from nft formation time, instead of the actual time at which token was staked
// leading to extra rewards (more than 1 LoveToken)
uint256 rewardsClaimed = loveToken.balanceOf(soulmate1) - balanceBeforeClaim;
assert(rewardsClaimed > 1e18);
​
// as it considers the time from relationship formation instead of actual staking time
// therefore total token awarded will be (weeks into relationship * 1 LoveToken)
uint256 weeksInRelation = (block.timestamp - relationshipCreationTime) / 1 weeks;
assertEq(rewardsClaimed, weeksInRelation * 1e18);
​
console2.log("Expected Rewards ->", 1e18, "LoveToken");
console2.log("Actual Rewareds ->", rewardsClaimed, "LoveToken");
}

Tools Used

Manual Review, Unit Test in Foundry

Recommendations

Instead of considering staking time with respect to NFT creation timestamp, consider the time at which the LoveToken was actually staked inside the Staking contract.

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.