UlyssesPool.setWeight() gets the logic wrong for the distribution of leftOverBandwidth.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesPool.sol#L223-L305

Vulnerability details

Impact

Detailed description of the impact of this finding.
setWeight() gets the logic wrong for the distribution of leftOverBandwidth. The main problem is that it gets the case of oldTotalWeights > newTotalWeights and the case of oldTotalWeights < newTotalWeights confused.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

UlyssesPool.setWeight() allows a user to set a new weight for an existing poolId. There are two cases:

1. oldTotalWeights < newTotalWeights, then some bandwith will be redistributed from the remaining bandwithStateLists to bandwidthStateList[poolIndex].bandwidth.

2. 1. oldTotalWeights > newTotalWeights, then some bandwith will be redistributed from bandwidthStateList[poolIndex].bandwidth to the remaining bandwithStateLists.

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesPool.sol#L223-L292

However, the implementation gets the logic wrong: it treats the first case as the second case and vice versa. For example, when oldTotalWeights > newTotalWeights, it uses the following logic:

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesPool.sol#L252C13-L262

Since oldTotalWeights > newTotalWeights, bandwidthStateList[i].bandwidth > oldBandwidth, as a result L260 will always revert due to underflow error. Therefore, the function setWeight() will revert as well.

Tools Used

VSCode

Recommended Mitigation Steps

We need to exchange the logic of the two cases.

 /// @inheritdoc IUlyssesPool
    function setWeight(uint256 poolId, uint8 weight) external nonReentrant onlyOwner {
        if (weight == 0) revert InvalidWeight();

        uint256 poolIndex = destinations[poolId];

        if (poolIndex == 0) revert NotUlyssesLP();

        uint256 oldRebalancingFee;

        for (uint256 i = 1; i < bandwidthStateList.length; i++) {
            uint256 targetBandwidth = totalSupply.mulDiv(bandwidthStateList[i].weight, totalWeights);

            oldRebalancingFee += _calculateRebalancingFee(bandwidthStateList[i].bandwidth, targetBandwidth, false);
        }

        uint256 oldTotalWeights = totalWeights;
        uint256 weightsWithoutPool = oldTotalWeights - bandwidthStateList[poolIndex].weight;
        uint256 newTotalWeights = weightsWithoutPool + weight;
        totalWeights = newTotalWeights;

        if (totalWeights > MAX_TOTAL_WEIGHT || oldTotalWeights == newTotalWeights) {
            revert InvalidWeight();
        }

        uint256 leftOverBandwidth;

        BandwidthState storage poolState = bandwidthStateList[poolIndex];
        poolState.weight = weight;

-        if (oldTotalWeights > newTotalWeights) {
+        if (oldTotalWeights < newTotalWeights) {

            for (uint256 i = 1; i < bandwidthStateList.length;) {
                if (i != poolIndex) {
                    uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
                    if (oldBandwidth > 0) {
                        bandwidthStateList[i].bandwidth =
                            oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();

                        leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
                    }
                }

                unchecked {
                    ++i;
                }
            }

            poolState.bandwidth += leftOverBandwidth.toUint248();
        } else {
            uint256 oldBandwidth = poolState.bandwidth;
            if (oldBandwidth > 0) {
                poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();

                leftOverBandwidth += oldBandwidth - poolState.bandwidth;
            }

            for (uint256 i = 1; i < bandwidthStateList.length;) {
                if (i != poolIndex) {
                    if (i == bandwidthStateList.length - 1) {
                        bandwidthStateList[i].bandwidth += leftOverBandwidth.toUint248();
                    } else if (leftOverBandwidth > 0) {
                        bandwidthStateList[i].bandwidth +=
                            leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
                    }
                }

                unchecked {
                    ++i;
                }
            }
        }

        uint256 newRebalancingFee;

        for (uint256 i = 1; i < bandwidthStateList.length; i++) {
            uint256 targetBandwidth = totalSupply.mulDiv(bandwidthStateList[i].weight, totalWeights);

            newRebalancingFee += _calculateRebalancingFee(bandwidthStateList[i].bandwidth, targetBandwidth, false);
        }

        if (oldRebalancingFee < newRebalancingFee) {
            asset.safeTransferFrom(msg.sender, address(this), newRebalancingFee - oldRebalancingFee);
        }
    }

Assessed type

Math

[c4-judge]

trust1995 marked the issue as duplicate of #766

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to 3 (High Risk)