Maia DAO Ecosystem - Atree's results

Lines of code

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

Vulnerability details

Impact

In function removeAsset, assetId is updated incorrectly.

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;
    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)));
}

function addAsset(address asset, uint256 _weight) external nonReentrant onlyOwner {
    if (assetId[asset] != 0) revert AssetAlreadyAdded();
    require(ERC20(asset).decimals() == 18);
    require(_weight > 0);

    assetId[asset] = assets.length + 1;
    assets.push(asset);
    weights.push(_weight);
    totalWeights += _weight;

    emit AssetAdded(asset, _weight);

    updateAssetBalances();
}

Proof of Concept

assetId should always be equal to assetIndex + 1, as zero represents that asset does not exist.

Tools Used

Manual

Recommended Mitigation Steps

assetId[lastAsset] = assetIndex + 1;

Assessed type

CanAuto