0xAsen - Votes across different periods are accrued for reward calculations instead of kept separately for each period

[sherlock-admin4]

0xAsen

High

Votes across different periods are accrued for reward calculations instead of kept separately for each period

Summary

Votes across different periods are accrued for reward calculations instead of kept separately for each period

Vulnerability Detail

The amounts.update function calculates the rewards based on the deposited amount. It's used in BribeRewarder::_modify like that:

(uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply,) = amounts.update(tokenId, deltaAmount);

If a user deposits 100 tokens for period1 and 200 tokens for period2, they should receive rewards for 100 tokens for period1 and rewards for 200 tokens for period2.

However, the current implementation adds the new deposit to the previous total deposit amount, which affects the reward calculation.

    function update(Parameter storage amounts, bytes32 key, int256 deltaAmount)
        internal
        returns (uint256 oldAmount, uint256 newAmount, uint256 oldTotalAmount, uint256 newTotalAmount)
    {   
        oldAmount = amounts.amounts[key];
        oldTotalAmount = amounts.totalAmount;

        //if the deltaAmount is 0
        if (deltaAmount == 0) {
            newAmount = oldAmount;
            newTotalAmount = oldTotalAmount;
        } else {
            newAmount = oldAmount.addDelta(deltaAmount);
            newTotalAmount = oldTotalAmount.addDelta(deltaAmount);
            amounts.amounts[key] = newAmount;
            amounts.totalAmount = newTotalAmount;
        }
    }

Scenario:

• User deposits 100 tokens in Period 1.
• The same user deposits 200 tokens in Period 2. The modify function adds these 200 tokens to the user's previous deposit of 100 tokens, making the total 300 tokens.

When rewards are calculated, they are incorrectly based on the total of 300 tokens instead of 200 tokens for User2 and 100 tokens for User1.

Impact

The issue could lead to incorrect reward allocation, potentially allowing users to receive more rewards than they should. This discrepancy can result in financial losses for the protocol and unfair advantages for certain users.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/libraries/Amounts.sol#L68

Tool used

Manual Review

Recommendation

The newAmount parameter should be reset when starting a new period or should create a mapping that keeps the amounts for each period.

Duplicate of #114