Maia DAO Ecosystem - KupiaSec's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/d0ebf632cf8caddb21e39d49ae15b18245319cd2/src/ulysses-amm/UlyssesToken.sol#L72

Vulnerability details

Impact

The asset removal logic would be broken as it tracks the wrong assetId after a removal.

Proof of Concept

The owner can remove an asset using removeAsset() and it updates the assetId mapping inside the function.

    function removeAsset(address asset) external nonReentrant onlyOwner {
        // No need to check if index is 0, it will underflow and revert if it is 0
        uint256 assetIndex = assetId[asset] - 1;

        uint256 newAssetsLength = assets.length - 1;

        if (newAssetsLength == 0) revert CannotRemoveLastAsset();

        totalWeights -= weights[assetIndex];

        address lastAsset = assets[newAssetsLength];

        assetId[lastAsset] = assetIndex; //@audit should be assetIndex + 1
        assets[assetIndex] = lastAsset;
        weights[assetIndex] = weights[newAssetsLength];

        assets.pop();
        weights.pop();
        assetId[asset] = 0;

        emit AssetRemoved(asset);

        updateAssetBalances();

        asset.safeTransfer(msg.sender, asset.balanceOf(address(this)));
    }

In this protocol, valid asset ids are started from 1 but it sets the raw assetIndex after the removal. After that, the asset removals wouldn't work properly.

1. Let's suppose assets = [A, B, C], assetId[A] = 1, assetId[B] = 2, assetId[C] = 3.
2. The admin calls removeAsset(A).
3. Then assets = [B, C], assetId[B] = 2, assetId[C] = 0 because assetIndex == 0 inside the function.
4. After that, the C asset can't be removed because assetId[C] = 0 and it will be considered as an unadded asset.

Tools Used

Manual Review

Recommended Mitigation Steps

removeAsset() should be modified like this.

assetId[lastAsset] = assetIndex + 1;

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/d0ebf632cf8caddb21e39d49ae15b18245319cd2/src/erc-4626/ERC4626MultiToken.sol#L100 https://github.com/code-423n4/2023-05-maia/blob/d0ebf632cf8caddb21e39d49ae15b18245319cd2/src/erc-4626/ERC4626MultiToken.sol#L138

Vulnerability details

Impact

Users might lose assets when they use deposit() or withdraw().

Proof of Concept

This multitoken works with several assets that have different weights and users can mint shares by depositing assets.

    function deposit(uint256[] calldata assetsAmounts, address receiver)
        public
        virtual
        nonReentrant
        returns (uint256 shares)
    {
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(assetsAmounts)) != 0, "ZERO_SHARES"); //@audit possible loss of assets

        // Need to transfer before minting or ERC777s could reenter.
        receiveAssets(assetsAmounts);

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assetsAmounts, shares);

        afterDeposit(assetsAmounts, shares);
    }

When users call deposit() with a list of assets, it calculates the shares inside convertToShares().

    function convertToShares(uint256[] calldata assetsAmounts) public view virtual returns (uint256 shares) {
        uint256 _totalWeights = totalWeights;
        uint256 length = assetsAmounts.length;

        if (length != assets.length) revert InvalidLength();

        shares = type(uint256).max;
        for (uint256 i = 0; i < length;) {
            uint256 share = assetsAmounts[i].mulDiv(_totalWeights, weights[i]);
            if (share < shares) shares = share;
            unchecked {
                i++;
            }
        }
    }

convertToShares() returns the minimum share among all shares for assets. But the asset weights might be changed by the owner and users might lose funds unexpectedly.

1. Let's suppose the multi-token has 2 assets A and B. weights[A] = 1, weights[B] = 1, totalWeights = 2
2. Alice wanted to get 100 shares and called deposit() with 50 amounts of A and B.
3. But before that, the asset weights were changed by the owner using setWeights(). So weights[A] = 2, weights[B] = 1, totalWeights = 3.
4. Then the calculated shares for A will be 50 * 3 / 2 = 75. 50 * 3 / 1 = 150 for B. So Alice will get min(75, 150) = 75 shares instead of 100.
5. Alice could get 75 shares with 50A and 25B and it means Alice loses 25B.

Similarly, users might burn more shares than they expected after asset weights are changed with withdraw().

Tools Used

Manual Review

Recommended Mitigation Steps

I think we should add a refund logic to deposit()/withdraw() functions so users can get back the unused assets.

Assessed type

ERC4626