Calling `BaseV2Gauge.detachUser` function does not update user's `getUserBoost` when it should be updated

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/53c7fe9d5e55754960eafe936b6cb592773d614c/src/gauges/BaseV2Gauge.sol#L106-L108 https://github.com/code-423n4/2023-05-maia/blob/62f4f01a522dcbb4b9abfe2f6783bbb67c0da022/src/erc-20/ERC20Boost.sol#L138-L143 https://github.com/code-423n4/2023-05-maia/blob/62f4f01a522dcbb4b9abfe2f6783bbb67c0da022/src/erc-20/ERC20Boost.sol#L150-L172 https://github.com/code-423n4/2023-05-maia/blob/7492906aff17e3ee8d7773ba5bf51a171051e401/src/talos/boost-aggregator/BoostAggregator.sol#L172-L177 https://github.com/code-423n4/2023-05-maia/blob/62f4f01a522dcbb4b9abfe2f6783bbb67c0da022/src/erc-20/ERC20Boost.sol#L198-L200 https://github.com/code-423n4/2023-05-maia/blob/62f4f01a522dcbb4b9abfe2f6783bbb67c0da022/src/erc-20/ERC20Boost.sol#L203-L227

Vulnerability details

Impact

Calling the following BaseV2Gauge.detachUser function only calls the ERC20Boost.detach function below without calling the ERC20Boost.updateUserBoost function below. Although the corresponding gauge is removed from the user's _userGauges and all of the user's boost for such gauge are deleted from the corresponding getUserGaugeBoost, not calling the ERC20Boost.updateUserBoost function causes the user's getUserBoost to not be updated.

https://github.com/code-423n4/2023-05-maia/blob/53c7fe9d5e55754960eafe936b6cb592773d614c/src/gauges/BaseV2Gauge.sol#L106-L108

    function detachUser(address user) external onlyStrategy {
        hermesGaugeBoost.detach(user);
    }

https://github.com/code-423n4/2023-05-maia/blob/62f4f01a522dcbb4b9abfe2f6783bbb67c0da022/src/erc-20/ERC20Boost.sol#L138-L143

    function detach(address user) external {
        require(_userGauges[user].remove(msg.sender));
        delete getUserGaugeBoost[user][msg.sender];

        emit Detach(user, msg.sender);
    }

https://github.com/code-423n4/2023-05-maia/blob/62f4f01a522dcbb4b9abfe2f6783bbb67c0da022/src/erc-20/ERC20Boost.sol#L150-L172

    function updateUserBoost(address user) external {
        uint256 userBoost = 0;

        address[] memory gaugeList = _userGauges[user].values();

        uint256 length = gaugeList.length;
        for (uint256 i = 0; i < length;) {
            address gauge = gaugeList[i];

            if (!_deprecatedGauges.contains(gauge)) {
                uint256 gaugeBoost = getUserGaugeBoost[user][gauge].userGaugeBoost;

                if (userBoost < gaugeBoost) userBoost = gaugeBoost;
            }

            unchecked {
                i++;
            }
        }
        getUserBoost[user] = userBoost;

        emit UpdateUserBoost(user, userBoost);
    }

This is unlike the following BoostAggregator.withdrawGaugeBoost function, which calls the ERC20Boost.decrementAllGaugesBoost function below and the ERC20Boost.updateUserBoost function. Besides calling the ERC20Boost.decrementAllGaugesBoost function, which further calls the ERC20Boost.decrementGaugesBoostIndexed function that can remove the corresponding gauge from the user's _userGauges and delete all of the user's boost for such gauge from the corresponding getUserGaugeBoost too, the ERC20Boost.updateUserBoost function is also called to update the user's getUserBoost accordingly. Because the ERC20Boost.updateUserBoost function is called in the BoostAggregator.withdrawGaugeBoost function, calling the BoostAggregator.withdrawGaugeBoost function does update the user's getUserBoost correctly.

https://github.com/code-423n4/2023-05-maia/blob/7492906aff17e3ee8d7773ba5bf51a171051e401/src/talos/boost-aggregator/BoostAggregator.sol#L172-L177

    function withdrawGaugeBoost(address to, uint256 amount) external onlyOwner {
        /// @dev May run out of gas.
        hermesGaugeBoost.decrementAllGaugesBoost(amount);
        hermesGaugeBoost.updateUserBoost(address(this));
        address(hermesGaugeBoost).safeTransfer(to, amount);
    }

https://github.com/code-423n4/2023-05-maia/blob/62f4f01a522dcbb4b9abfe2f6783bbb67c0da022/src/erc-20/ERC20Boost.sol#L198-L200

    function decrementAllGaugesBoost(uint256 boost) external {
        decrementGaugesBoostIndexed(boost, 0, _userGauges[msg.sender].length());
    }

https://github.com/code-423n4/2023-05-maia/blob/62f4f01a522dcbb4b9abfe2f6783bbb67c0da022/src/erc-20/ERC20Boost.sol#L203-L227

    function decrementGaugesBoostIndexed(uint256 boost, uint256 offset, uint256 num) public {
        address[] memory gaugeList = _userGauges[msg.sender].values();

        uint256 length = gaugeList.length;
        for (uint256 i = 0; i < num && i < length;) {
            address gauge = gaugeList[offset + i];

            GaugeState storage gaugeState = getUserGaugeBoost[msg.sender][gauge];

            if (_deprecatedGauges.contains(gauge) || boost >= gaugeState.userGaugeBoost) {
                require(_userGauges[msg.sender].remove(gauge)); // Remove from set. Should never fail.
                delete getUserGaugeBoost[msg.sender][gauge];

                emit Detach(msg.sender, gauge);
            } else {
                gaugeState.userGaugeBoost -= boost.toUint128();

                emit DecrementUserGaugeBoost(msg.sender, gauge, gaugeState.userGaugeBoost);
            }

            unchecked {
                i++;
            }
        }
    }

In comparison, since the ERC20Boost.updateUserBoost function is not called after executing hermesGaugeBoost.detach(user) in the BaseV2Gauge.detachUser function, calling the BaseV2Gauge.detachUser function does not update the user's getUserBoost at all. If the user's boost for the gauge to be detached was the highest among all gauges that were attached for the user, the user's getUserBoost should become lower after detaching such gauge but the user's getUserBoost remains the same when the BaseV2Gauge.detachUser function is called, which is an accounting error for the user's getUserBoost.

Proof of Concept

The following steps can occur for the described scenario.

1. Alice's boost for Gauge 1 is the highest among all gauges that were attached for her.
2. The BaseV2Gauge.detachUser function is called to detach Gauge 1 for Alice.
3. Gauge 1 is removed from Alice's _userGauges, and all of Alice's boost for Gauge 1 are deleted from the corresponding getUserGaugeBoost.
4. Alice's getUserBoost is not updated and still remains the same, which equals Alice's old boost for Gauge 1, incorrectly.

Tools Used

VSCode

Recommended Mitigation Steps

The BaseV2Gauge.detachUser function can be updated to call the ERC20Boost.updateUserBoost function with user being the input after executing hermesGaugeBoost.detach(user).

Assessed type

Other

[c4-judge]

trust1995 marked the issue as duplicate of #37

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)