`LockingMultiRewards` contract: users will be able to claim more rewards than they are entitled to by calling `withdrawWithRewards()`

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L186-L189

Vulnerability details

Impact

• LockingMultiRewards.withdrawWithRewards function enables users to withdraw an amount of their unlocked staked deposits and then claim their accumulated rewards in the same function:

    function withdrawWithRewards(uint256 amount) public virtual {
            withdraw(amount);
            _getRewards(msg.sender);
    }

where first the withdrawal of the unlocked staked amount is executed, but before that the accumulated rewards of the user are updated via _updateRewardsForUser(), then his unlocked balance is decreased by the withdrawn amount:

          function withdraw(uint256 amount) public virtual {
                  if (amount == 0) {
                      revert ErrZeroAmount();
                  }

                  _updateRewardsForUser(msg.sender);

                  _balances[msg.sender].unlocked -= amount;
                  unlockedSupply -= amount;

                  stakingToken.safeTransfer(msg.sender, amount);
                  stakingTokenBalance -= amount;

                  emit LogWithdrawn(msg.sender, amount);
              }

    function _updateRewardsForUser(address user) internal {
        uint256 balance = balanceOf(user); // << unlocked + locked*3
        uint256 totalSupply_ = totalSupply(); //<< unlocked + locked*3

        for (uint256 i; i < rewardTokens.length; ) {
            address token = rewardTokens[i];
            _udpateUserRewards(user, balance, token, _updateRewardsGlobal(token, totalSupply_));

            unchecked {
                ++i;
            }
        }
    }

then after the withdrawal is executed, the _getRewards() function is invoked to transfer the rewards for the user.

• But as can be noticed, the _getRewards() function is invoked directly after the withdraw() function without updating the user rewards based on his new balance after the withdrawal, and this will result in the user claiming more rewards than he is entitled to, because his old balance before withdrawal is the one used to calculate his accumulated rewards.

• To assure that this is a vulnerability/missing update: the getRewards() function that is intended to send the accumulated rewards to the user updates the rewards of the user based on his current balance (locked and unlocked) before sending them.

Proof of Concept

LockingMultiRewards.withdrawWithRewards function

    function withdrawWithRewards(uint256 amount) public virtual {
        withdraw(amount);
        _getRewards(msg.sender);
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Update withdrawWithRewards() function to update user's uncalimed rewards based on his current balance (similar mechanism to the getRewards() function):

    function withdrawWithRewards(uint256 amount) public virtual {
        withdraw(amount);
+       _updateRewardsForUser(msg.sender);
        _getRewards(msg.sender);
    }

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[141345]

the mitigation makes no diff

[c4-judge]

thereksfour marked the issue as unsatisfactory: Insufficient proof

[DevHals]

Hi @thereksfour ,

this issue clearly states that withdrawing and claiming rewards via withdrawWithRewards() will result in claiming more rewards as it uses the old balance of the user before he withdrew:

• calling withdrawWithRewards will first update used rewards based on his original balance
• then the amount will be withdrawn where the unlocked user balance will be decreased

• then the rewards will be claimed based on the old unlocked balance instead of the new unlocked balance that has been decreased as the function misses _updateRewardsForUser before claiming rewards

  Could you please have a second look an re-evaluate? Thanks!

[thereksfour]

Invalid, the added _updateRewardsForUser in the recommendation will do nothing, because userRewardPerTokenPaid has been updated in withdraw, which makes the _earned of _updateRewardsForUser in the recommendation return 0. Also, if you disagree, please provide a step-by-step POC to illustrate the issue.