bin2chen - stake() missing set lastEpochClaimed when userBalance equal 0

[github-actions[bot]]

bin2chen

medium

stake() missing set lastEpochClaimed when userBalance equal 0

Summary

because stake() don't set lastEpochClaimed[user] = last epoch if userBalance equal 0 So all new stake user must loop from 0 to last epoch for _claimRewards() As the epoch gets bigger and bigger it will waste a lot of GAS, which may eventually lead to GAS_OUT

Vulnerability Detail

in stake(), when the first-time stake() only rewardsPerTokenClaimed[msg.sender] but don't set lastEpochClaimed[msg.sender]

    function stake(
        uint256 amount_,
        bytes calldata proof_
    ) external nonReentrant requireInitialized updateRewards tryNewEpoch {
...
        uint256 userBalance = stakeBalance[msg.sender];
        if (userBalance > 0) {
            // Claim outstanding rewards, this will update the rewards per token claimed
            _claimRewards();
        } else {
            // Initialize the rewards per token claimed for the user to the stored rewards per token
@>          rewardsPerTokenClaimed[msg.sender] = rewardsPerTokenStored;
        }

        // Increase the user's stake balance and the total balance
        stakeBalance[msg.sender] = userBalance + amount_;
        totalBalance += amount_;

        // Transfer the staked tokens from the user to this contract
        stakedToken.safeTransferFrom(msg.sender, address(this), amount_);
    }

so every new staker , needs claims from 0

    function _claimRewards() internal returns (uint256) {
        // Claims all outstanding rewards for the user across epochs
        // If there are unclaimed rewards from epochs where the option token has expired, the rewards are lost

        // Get the last epoch claimed by the user
@>      uint48 userLastEpoch = lastEpochClaimed[msg.sender];

        // If the last epoch claimed is equal to the current epoch, then only try to claim for the current epoch
        if (userLastEpoch == epoch) return _claimEpochRewards(epoch);

        // If not, then the user has not claimed all rewards
        // Start at the last claimed epoch because they may not have completely claimed that epoch
        uint256 totalRewardsClaimed;
@>     for (uint48 i = userLastEpoch; i <= epoch; i++) {
            // For each epoch that the user has not claimed rewards for, claim the rewards
            totalRewardsClaimed += _claimEpochRewards(i);
        }

        return totalRewardsClaimed;
    }

With each new addition of epoch, the new stake must consumes a lot of useless loops, from loop 0 to last epoch When epoch reaches a large size, it will result in GAS_OUT and the method cannot be executed

Impact

When the epoch gradually increases, the new take will waste a lot of GAS When it is very large, it will cause GAS_OUT

Code Snippet

https://github.com/sherlock-audit/2023-06-bond/blob/main/options/src/fixed-strike/liquidity-mining/OTLM.sol#L324-L327

Tool used

Manual Review

Recommendation

    function stake(
        uint256 amount_,
        bytes calldata proof_
    ) external nonReentrant requireInitialized updateRewards tryNewEpoch {
...
        if (userBalance > 0) {
            // Claim outstanding rewards, this will update the rewards per token claimed
            _claimRewards();
        } else {
            // Initialize the rewards per token claimed for the user to the stored rewards per token
            rewardsPerTokenClaimed[msg.sender] = rewardsPerTokenStored;
+           lastEpochClaimed[msg.sender] = epoch;
        }

[Oighty]

Agree with the proposed solution.

[ctf-sec]

Great finding, agree with medium severity

[Oighty]

Fix implemented at https://github.com/Bond-Protocol/options/pull/5

[ctf-sec]

Will look into this, seems all the duplicate suggest the fix:

 lastEpochClaimed[msg.sender] = epoch;

but the implemented fix is

 lastEpochClaimed[msg.sender] = epoch -1

maybe testing can help as well, just want to make sure there is no off-by-one issue o(╥﹏╥)o

[Oighty]

Will look into this, seems all the duplicate suggest the fix:

 lastEpochClaimed[msg.sender] = epoch;

but the implemented fix is

 lastEpochClaimed[msg.sender] = epoch -1

maybe testing can help as well, just want to make sure there is no off-by-one issue o(╥﹏╥)o

The reason to set lastEpochClaimed to epoch - 1 is that you want the user state to appear like they have claimed everything before the epoch they started staking on. They haven't claimed any tokens for the current epoch yet, so that would be inaccurate.

[Oighty]

@ctf-sec did you have a chance to review this more?

[ctf-sec]

Yes, fix looks good