First depositor would encounter unexpected behaviours and might not receive any rewards

[codehawks-bot]

First depositor would encounter unexpected behaviours and might not receive any rewards

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Staking.sol#L36-L42

Summary

In the provided Staking contract's deposit function, the sequence of operations results in the first depositor being unable to receive any rewards. The issue arises due to the order of operations in the deposit function, where the updateFor function is called before the user's balance is updated. This, in combination with conditions in the update and updateFor functions, results in the first depositor's shares not being added.

Vulnerability Details

In the deposit function, the contract calls updateFor(msg.sender) before adding the deposited amount to balances[msg.sender]. The updateFor function, in turn, calls the update function. The update function has a condition to only perform an update if _balance > balance. However, for the first depositor, _balance would be zero, as there are no rewards in the contract yet. Similarly, balance would also be zero, as it's defaulted to zero in the contract. So, no update would be made to the contract's index or balance in this case.

When execution returns to the updateFor function, it also has a condition to update only if (_supplied > 0). But for the first depositor, _supplied would also be zero because the balance is updated in the deposit function only after calling updateFor. As a result, the execution wouldn't reach the line where the user's shares are added, causing the first depositor to not receive any rewards.

Here is the deposit() function:

function deposit(uint _amount) external {
    TKN.transferFrom(msg.sender, address(this), _amount);
    updateFor(msg.sender); // called before updating the user's balance
    balances[msg.sender] += _amount; // @audituser's balance updated after `updateFor`
}

Impact

The first depositor possibly missing out on rewards can have a significant impact on the fairness and appeal of the staking system. It also creates a potentially unintended imbalance in reward distribution.

Tools Used

Manual Audit

Recommended Mitigation

Pretty interesting case, but the recommended solution imo is to move the updateFor(msg.sender) function call to after the user's balance is updated in the deposit function. This ensures that the user's balance and the state of the contract are updated correctly before attempting to distribute rewards.

The corrected deposit function could look like this:

function deposit(uint _amount) external {
    TKN.transferFrom(msg.sender, address(this), _amount);
    balances[msg.sender] += _amount; // move this before the `updateFor`
    updateFor(msg.sender); // move this after updating the user's balance
}

With this change, I think the first depositor should now be correctly accounted for.

[PatrickAlphaC]

The impact of this is HIGH, but I argue likelihood is LOW, I'm moving to medium.

[qckhp96565463]

[ESCALATE] I think this is the duplicate of https://github.com/Cyfrin/2023-07-beedle/issues/757. See Recommend Mitigation for both.

[kosedogus]

This issue is invalid. Index is initialized as 0. When WETH transferred to the contract user can call claim() which will first update index, then update user's claimable (because index has changed, it won't be 0), and then transfer rewards to user.

[PatrickAlphaC]

I think I agree with @qckhp96565463.