UlyssesToken.setWeights(...) can cause user loss of assets on vault deposits/withdrawals

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/ERC4626MultiToken.sol#L200-L207
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/ERC4626MultiToken.sol#L250-L256

Vulnerability details

Impact

The ERC-4626 paradigm of deposit/mint and withdraw/redeem, where a single underlying asset amount can always be converted to a number of vault shares and vice-versa, breaks as soon as there are multiple weighted underlying assets involved.
While it's easy to convert from shares to asset amounts using the weight factors, it's hard to convert from asset amounts to shares in case they are not exactly distributed according to the weight factors.

In the Ulysses protocol this was solved the following way:

• On UlyssesToken.deposit(...) every asset amount is converted to shares and the smallest of them is the one received for the deposit, see ERC4626MultiToken.convertToShares(...). As a consequence, excess assets provided on deposit are lost for the user and cannot be redeemed with the received shares.
• On UlyssesToken.withdraw(...) every asset amount is converted to shares and the greatest of them is the one required to withdraw the given asset amounts, see ERC4626MultiToken.previewWithdraw(...). As a consequence, less assets than entitled to according to the share count can be withdrawn from the vault incuring a loss for the user.

One might argue that this issue is of low severity due to user error and the user being responsible to only use asset amounts in accordance with the vault's asset weights. However, the asset weights are not fixed and can be changed at any time by the ower of the UlyssesToken contract via the setWeights(...) method. That is what makes this an actual issue.
Consider the scenario when a user is about to deposit/withdraw assets not knowing their respective weights have changed, or even worse the deposit/withdraw transaction is already in the mempool but the call to setWeights(...) is executed before. Depending on the new asset weights, this will inevitably lead to a loss for the user.

Proof of Concept

The following in-line documented PoC demonstrates the above claims for the deposit case. Just add the new test case below to test/ulysses-amm/UlyssesTokenTest.t.sol:InvariantUlyssesToken and run it with forge test -vv --match-test test_UlyssesToken.

function test_UlyssesTokenSetWeightsDepositLoss() public {
    UlyssesToken token = UlyssesToken(_vault_);

    // initialize asset amounts according to weights, mint tokens & give approval to UlyssesToken vault
    uint256[] memory assetsAmounts = new uint256[](NUM_ASSETS);
    for (uint256 i = 0; i < NUM_ASSETS; i++) {
        assetsAmounts[i] = 1000 * token.weights(i);
        MockERC20(token.assets(i)).mint(address(this), 1e18);
        MockERC20(token.assets(i)).approve(address(token), 1e18);
    }

    // deposit assets & check if we got the expected number of shares
    uint256 expectedShares = token.previewDeposit(assetsAmounts);
    uint256 receivedShares = token.deposit(assetsAmounts, address(this));
    assertEq(expectedShares, receivedShares); // OK

    // check if we can redeem the same asset amounts as we deposited
    uint256[] memory redeemAssetsAmounts = token.previewRedeem(receivedShares);
    assertEq(assetsAmounts, redeemAssetsAmounts); // OK

    // assuming everything is fine, we submit another deposit transaction to the mempool
    // meanwhile the UlyssesToken owner changes the asset weights
    uint256[] memory weights = new uint256[](NUM_ASSETS);
    for (uint256 i = 0; i < NUM_ASSETS; i++) {
        weights[i] = token.weights(i);
    }
    weights[0] *= 2; // double the weight of first asset
    token.setWeights(weights);

    // now our deposit transaction gets executed, but due to the changed asset weights
    // we got less shares than expected while sending too many assets (except for asset[0])
    receivedShares = token.deposit(assetsAmounts, address(this));
    assertEq(expectedShares, receivedShares, "got less shares than expected");

    // due to the reduced share amount we cannot redeem all the assets we deposited,
    // we lost the excess assets we have deposited (except for asset[0])
    redeemAssetsAmounts = token.previewRedeem(receivedShares);
    assertEq(assetsAmounts, redeemAssetsAmounts, "can redeem less assets than deposited");
}

The test case shows that less shares than expected are received in case of changed weights and any deposited excess assets cannot be redeemed anymore:

Running 1 test for test/ulysses-amm/UlyssesTokenTest.t.sol:InvariantUlyssesToken
[FAIL. Reason: Assertion failed.] test_UlyssesTokenSetWeightsDepositLoss() (gas: 631678)
Logs:
  Error: got less shares than expected
  Error: a == b not satisfied [uint]
        Left: 45000
       Right: 27500
  Error: can redeem less assets than deposited
  Error: a == b not satisfied [uint[]]
        Left: [10000, 10000, 20000, 5000]
       Right: [10000, 5000, 10000, 2500]

For the sake of simplicity, the test for the withdrawal case was skipped since it's exactly the same problem just in the reverse direction.

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

• On UlyssesToken.deposit(...), only transfer the necessary token amounts (according to the computed share count) from the sender, like the UlyssesToken.mint(...) method does.
• On UlyssesToken.withdraw(...), transfer all the asset amounts the sender is entitled to (according to the computed share count) to the receiver, like the UlyssesToken.redeem(...) method does.

Assessed type

Rug-Pull

[trust1995]

The time-sensitivity consideration seems to be valid.

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-sponsor]

0xLightt marked the issue as sponsor disputed

[0xLightt]

Hey, this is intended, the goal is that the user gets the same number of assets, but can be in a different ratio according to weights. That is reason behind the first failing statement. The second failed statement is because you are passing the incorrect shared obtained by the incorrect assetsAmounts array.
This is a working version of the test passing all tests:

    function test_UlyssesTokenSetWeightsDepositLoss() public {
        UlyssesToken token = UlyssesToken(_vault_);

        // initialize asset amounts according to weights, mint tokens & give approval to UlyssesToken vault
        uint256[] memory assetsAmounts = new uint256[](NUM_ASSETS);
        for (uint256 i = 0; i < NUM_ASSETS; i++) {
            assetsAmounts[i] = 1 ether * token.weights(i);
            MockERC20(token.assets(i)).mint(address(this), 100 ether);
            MockERC20(token.assets(i)).approve(address(token), 100 ether);
        }

        // deposit assets & check if we got the expected number of shares
        uint256 expectedShares = token.previewDeposit(assetsAmounts);
        uint256 receivedShares = token.deposit(assetsAmounts, address(this));
        assertEq(expectedShares, receivedShares); // OK

        // check if we can redeem the same asset amounts as we deposited
        uint256[] memory redeemAssetsAmounts = token.previewRedeem(receivedShares);
        assertEq(assetsAmounts, redeemAssetsAmounts); // OK

        // assuming everything is fine, we submit another deposit transaction to the mempool
        // meanwhile the UlyssesToken owner changes the asset weights
        uint256[] memory weights = new uint256[](NUM_ASSETS);
        for (uint256 i = 0; i < NUM_ASSETS; i++) {
            weights[i] = token.weights(i);
        }
        weights[0] *= 2; // double the weight of first asset
        token.setWeights(weights);

        // due to the reduced share amount we cannot redeem all the assets we deposited,
        // we lost the excess assets we have deposited (except for asset[0])
        redeemAssetsAmounts = token.previewRedeem(expectedShares);
        uint256 expectedSum;
        uint256 sum;
        for (uint256 i = 0; i < NUM_ASSETS; i++) {
            expectedSum += assetsAmounts[i];
            sum += redeemAssetsAmounts[i];
        }
        assertApproxEqAbs(expectedSum, sum, 1, "can redeem less assets than deposited");

        // now our deposit transaction gets executed, but due to the changed asset weights
        // we got less shares than expected while sending too many assets (except for asset[0])
        receivedShares = token.deposit(redeemAssetsAmounts, address(this));
        assertApproxEqAbs(expectedShares, receivedShares, 1, "got less shares than expected");
    }

[c4-judge]

trust1995 marked the issue as unsatisfactory:
Invalid

[MarioPoneder]

@trust1995
Providing some additional context:

• The sponsor has shown in his version of the test case that the impermanent loss due to re-weighting is intentional and works correctly. This is typical behaviour for mulit-asset vaults and does not invalidate the original issue in any way.
• The real problem is the race condition, which was correctly assessed by the judge as "time-sensitivity consideration", that causes undesired user loss (involuntary donation of assets) in case of a transaction order of previewDeposit -> setWeights -> deposit. (There is a related race condition issue on withdrawal.)
• In the sponsor's test case: setWeights should be to moved between previewRedeem and deposit to replicate the original issue.

Appreciate everyone's efforts and have a nice day!

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as selected for report

[0xLightt]

We recognize the audit's findings on Ulysses Token. These will not be rectified due to the upcoming migration of this section to Balancer Stable Pools Wrapper.

[c4-sponsor]

0xLightt marked the issue as sponsor acknowledged

[c4-sponsor]

0xLightt marked the issue as sponsor confirmed