Target pool bandwidth incorrectly calculated when decreasing weight

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/ccc9a39240dbd8eab22299737370996b2b833efd/src/ulysses-amm/UlyssesPool.sol#L269

Vulnerability details

Impact

Target pool bandwidth calculation is incorrect, which leads to incorrect pool balancing.

Proof of Concept

In setWeights of UlyssesPool.sol, the leftOverBandwidth gets added to the target poolState.bandwidth. As discussed with the Sponsor, this is incorrect because when weight is decreased, the bandwidth should decrease to distribute to the other pools.

This PoC uses a new test file: UlyssesFactoryTest.t.sol. Run the test by calling forge test --match-test testReduceWeightIncreasesBandwidth

import {Test, StdUtils} from "forge-std/Test.sol";
import {UlyssesFactory} from "@ulysses-amm/factories/UlyssesFactory.sol";
import {UlyssesPool} from "@ulysses-amm/UlyssesPool.sol";
import {ERC20} from "solmate/tokens/ERC20.sol";
import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";

contract InvariantUlyssesFactory is Test {
    struct BandwidthState {
        uint248 bandwidth;
        uint8 weight;
        UlyssesPool destination;
    }
    UlyssesFactory ulyssesFactory;
    ERC20 token1;
    ERC20 token2;
    address alice = address(1);
    function setUp() public {
        ulyssesFactory = new UlyssesFactory(address(this));
        token1 = new MockERC20("Token1", "TK1", 18);
        token2 = new MockERC20("Token2", "TK2", 18);
        deal(address(token1), alice, 1 ether);
    }
    function testReduceWeightIncreasesBandwidth() public {
        ERC20[] memory assets = new ERC20[](4);
        assets[0] = token1;
        assets[1] = token1;
        assets[2] = token1;
        assets[3] = token1;

        uint8[][] memory weights = new uint8[][](4); // outer
        // Initialize the inner arrays
        weights[0] = new uint8[](4);
        weights[1] = new uint8[](4);
        weights[2] = new uint8[](4);
        weights[3] = new uint8[](4);

        // Pool1 weights
        weights[0][0] = 5;
        weights[0][1] = 10;
        weights[0][2] = 10;
        weights[0][3] = 10;

        // Pool2 weights
        weights[1][0] = 5;
        weights[1][1] = 10;
        weights[1][2] = 10;
        weights[1][3] = 10;

        // Pool3 weights
        weights[2][0] = 5;
        weights[2][1] = 10;
        weights[2][2] = 10;
        weights[2][3] = 10;

        // Pool4 weights
        weights[3][0] = 5;
        weights[3][1] = 10;
        weights[3][2] = 10;
        weights[3][3] = 10;
        
        
        uint256[] memory poolIds = ulyssesFactory.createPools(assets, weights, address(this));
        UlyssesPool pool1 = ulyssesFactory.pools(poolIds[0]); // id = 2

        vm.startPrank(alice);
        token1.approve(address(pool1), 0.1 ether);
        pool1.deposit(0.1 ether, alice);
        vm.stopPrank();
        
        uint256 pool1BandwidthBefore = pool1.getBandwidth(3);

        // Set weight to lower
        pool1.setWeight(3, 5);

        uint256 pool1BandwidthAfter = pool1.getBandwidth(3);
        assertEq(pool1BandwidthAfter > pool1BandwidthBefore, true);
    }
}

Tools Used

Manual

Recommended Mitigation Steps

Consider changing this line to:

poolState.bandwidth -= leftOverBandwidth.toUint248();

Assessed type

Math

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as duplicate of #772

[c4-judge]

trust1995 changed the severity to 3 (High Risk)

[c4-judge]

trust1995 marked the issue as duplicate of #766