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

`Staking.sol`: users lose their reward by calling `withdraw()` before `claimRewards()`

wiasliaw

Vulnerability Details

The calculation about the reward use the current state as the parameter. If user perform the withdraw, the current state will be zero in calling claimRewards() later.

Impact

The user might lose the whole of reward no matter how long they stake.

Tools Used

Manual.

Recommendations

  • add a state variable to track user's unclaimed reward

mapping(address => uint256) public unclaimReward;

  • add function to calculate reward

function _calcReward() internal returns (uint256 ret) {
uint256 timeInWeeksSinceLastClaim = ((block.timestamp - lastClaim[msg.sender]) / 1 weeks);
if (timeInWeeksSinceLastClaim != 0) {
lastClaim[msg.sender] = block.timestamp;
}
ret = userStakes[msg.sender] * timeInWeeksSinceLastClaim;
}

  • record the user's unclaimed reward before the change of staking token's balance in user-faced functions.

unclaimReward[msg.sender] = _calcReward();

PoC

Logs below. testClaimPoC perform withdraw before claiming reward. It shows that user lose the reward.

[PASS] testClaimGeneral() (gas: 418252)
Logs:
100000000000000000000
1500000000000000000000
​
[PASS] testClaimPoC() (gas: 378734)
Logs:
100000000000000000000
100000000000000000000
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;
​
import {Test, console2} from "forge-std/Test.sol";
​
import {BaseTest} from "./unit/BaseTest.t.sol";
​
contract Claim is BaseTest {
function testClaimGeneral() external {
uint256 balance = 100 ether;
_giveLoveTokenToSoulmates(balance);
​
vm.startPrank(soulmate1);
loveToken.approve(address(stakingContract), balance);
console2.log(loveToken.balanceOf(soulmate1));
stakingContract.deposit(balance);
stakingContract.claimRewards();
stakingContract.withdraw(balance);
console2.log(loveToken.balanceOf(soulmate1));
vm.stopPrank();
}
​
function testClaimPoC() external {
uint256 balance = 100 ether;
_giveLoveTokenToSoulmates(balance);
​
vm.startPrank(soulmate1);
loveToken.approve(address(stakingContract), balance);
console2.log(loveToken.balanceOf(soulmate1));
stakingContract.deposit(balance);
stakingContract.withdraw(balance); // withdraw before claim
stakingContract.claimRewards();
console2.log(loveToken.balanceOf(soulmate1));
vm.stopPrank();
}
}
Updates

Lead Judging Commences

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

finding-withdraw-before-claimReward

If we we implement a correct claimRewards function with its intended logic, this would indeed be an issue. I believe low severity for this findings and its duplicates to be appropriate given it is dependent on users lack of understanding of claiming rewards first before a withdrawal.

Support

FAQs

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