Maia DAO Ecosystem - zzzitron's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesToken.sol#L72 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesToken.sol#L45 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/ERC4626MultiToken.sol#L66-L69 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/ERC4626MultiToken.sol#L77-L80 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesToken.sol#L110-L118

Vulnerability details

Impact

Summary of the impact:

When the function UlyssesToken.removeAsset() is called, it is possible that the wrong assetId is assigned to an asset. As a result it is possible that:

1. A duplicate asset may be added.

2. When a duplicate asset would be added, the contract might hold less assets than it should hold, potentially resulting in insolvency or other issues.

3. An asset may not be able to be removed.

4. A wrong asset may be removed when UlyssesToken.removeAsset() is called next time.

5. The wrong assetId may be returned upon the UlyssesToken.assetId() view function.

The ERC4626MultiToken.assets and ERC4626MultiToken.weights arrays are crucial to determine which assets will be used. Based on ERC4626MultiToken.assets and ERC4626MultiToken.weights the UlyssesToken (which inherits ERC4626MultiToken) will pull and push the assets around, therefore when these arrays contain wrong information, the UlyssesToken will receive and give wrong assets. Therefore this issue is of high severity.

Detail of the impact:

When the function UlyssesToken.removeAsset() is called, the assetIndex is assigned to assetId[lastAsset] which is wrong:

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

This means that potentially a wrong assetId gets assigned to an asset.

Example for how a duplicate asset can be added:

• There are 3 assets: A, B, C in the ERC4626MultiToken.assets array

• Now UlyssesToken.removeAsset() is called with the address of asset A.

• The assetIndex of asset A is determined which is 0 (line 62 UlyssesToken.sol).

• The assetId of C is set to 0 (line 72 UlyssesToken.sol).

• C is then assigned to assets[0] on line 73 in UlyssesToken.sol.

• Now due to the fact that asset C has an assetId of 0, you can add asset C again via UlyssesToken.addAsset() as a duplicate. This will not revert, because the assetId of asset C is 0: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesToken.sol#L45

• Also, asset C can't be removed, because when calling UlyssesToken.removeAsset(), because C's assetId is 0, and the calculation on line 62 will revert due to an underflow.

Example for how a wrong asset might get removed accidentally:

• There are 3 assets: A, B, C in the ERC4626MultiToken.assets array

• Now UlyssesToken.removeAsset() is called with the address of asset B.

• The assetIndex of asset B is determined which is 1 (line 62 UlyssesToken.sol).

• The assetId of C is set to 1 (line 72 UlyssesToken.sol).

• C is then assigned to assets[1] on line 73 in UlyssesToken.sol.

• Now there is A and C in the assets array, but C has a wrong assetId.

• Now if you want to remove asset C by calling again UlyssesToken.removeAsset(), the assetId of C is 1. Therefore assetIndex for C is calculated to be 0 on line 62. Then on line 73 A is overwritten and removed with the asset C due to the wrong index. So basically you wanted to remove asset C, but asset A got removed.

Here are a few examples where the ERC4626MultiToken.assets and ERC4626MultiToken.weights arrays are used and where it is crucial that they are correct:

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/ERC4626MultiToken.sol#L66-L69

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/ERC4626MultiToken.sol#L77-L80

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

In these functions ERC4626MultiToken.assets information is used to receive and send out assets, which is why it is crucial that the ERC4626MultiToken.assets array has the correct information to avoid severe issues like transferring the wrong assets.

In the case that the ERC4626MultiToken.assets array contains duplicate assets, the UlyssesToken.updateAssetBalances() will transfer to match the last weight of the duplicated asset. For example, asset A is duplicated, and A's first weight is 50, and second weight is 10, then the UlyssesToken will hold only A with the weight of 10. Therefore when a user tries to withdraw, UlyssesToken might be in the position of lack of funds.

Proof of Concept

Here is a POC that proves that an asset can be added multiple times:

https://gist.github.com/zzzitron/8b7c0a2c6a6a76f247eceefd3c8e462f

Tools Used

Manual review

Recommended Mitigation Steps

Consider adjusting UlyssesToken.removeAsset() in the following way in order to avoid assigning a wrong assetId:

// UlyssesToken
// removeAsset
72        assetId[lastAsset] = assetId[asset];

Assessed type

ERC4626