0xboriskataa - Owner will not be able to change the weight of one of the old pools or remove it

[sherlock-admin4]

0xboriskataa

High

Owner will not be able to change the weight of one of the old pools or remove it

Summary

Voter.sol: setTopPoolIdsWithWeights() can be used by the owner to set the weight for each pool. If old pools are present it removes them and resets their weight to 0. However due to incorrect array itteration a revert can happen which will prevent the owner from overwriting the weight of one of the old pools. It also prevents him from removing it.

Vulnerability Detail

Here is part of the code for the setTopPoolIdsWithWeights():

    function setTopPoolIdsWithWeights(uint256[] calldata pids, uint256[] calldata weights) external {
        if (msg.sender != _operator) _checkOwner();

        uint256 length = pids.length;
        if (length != weights.length) revert IVoter__InvalidLength();

        uint256[] memory oldIds = _topPids.values();

        if (oldIds.length > 0) {
            // masterchef snould be updated beforehand

            for (uint256 i = oldIds.length; i > 0;) {
                uint256 pid = oldIds[--i];

                _topPids.remove(pid);
                _weights[pid] = 0;
            }
        }

        for (uint256 i; i < length; ++i) {
            uint256 pid = pids[i];
            if (!_topPids.add(pid)) revert IVoter__DuplicatePoolId(pid);
        }

First, if old pools are present it itterates through them in order to remove each one from _topPids and set their weight to 0. The issue is that the loop doesn't itterate through the element with index 0:

       for (uint256 i = oldIds.length; i > 0;) {
            uint256 pid = oldIds[--i];

            _topPids.remove(pid);
            _weights[pid] = 0;
        }

As we can see if i > 0 it will get into the for loop and remove the element with index of i. However if i = 0 it will not get inside the body of the loop and thus the 0th element will not be removed from _topPids.

This can cause an issue in the second for loop where we itterate through the new pools and try to add them in _topPids:

        for (uint256 i; i < length; ++i) {
            uint256 pid = pids[i];
            if (!_topPids.add(pid)) revert IVoter__DuplicatePoolId(pid);
        }

As you can see there is an if check and its purpose is to not allow duplicate pools.

Imagine this scenario:

1. Owner has added 5 pools to _topPids with their weights.
2. He wants to add new pools and adjust the weight of old ones.
3. He won't be able to adjust the weight of the first pool in _topPids because it cannot be removed from it as it has an index of 0. The transaction will simply revert because the if statement will catch duplicate pools.

Impact

Owner will not be able to change the weight of the first pool in the _topPids or remove it. _topPids represent farms that users can deposit in. The weight of each pool/farm affects how many rewards a user will get from it.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L271-L276

Tool used

Manual Review

Recommendation

-           for (uint256 i = oldIds.length; i > 0;) {
+           for (uint256 i = oldIds.length; i >= 0;) {
                uint256 pid = oldIds[--i];

                _topPids.remove(pid);
                _weights[pid] = 0;
            }

[0xSmartContract]

the claim that "it doesn't loop through 0" is entirely wrong. --i will decrement i first, and then apply any operations

PoC: Run this contract on remix IDE with n = 5

pragma solidity >=0.8.2 <0.9.0;

contract Test {
    function test(uint n) public pure returns(uint256 sum) {
        for (uint i = n; i > 0;) {
            sum += --i;       
        }
    }
}

Then the output is 10, or 0+1+2+3+4. If it iterated 1 2 3 4 5 then the sum should be 15

So , invalid