code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Proof of Concept

Currently, updateAssetBalance() will transfer zero tokens from the msg.sender to the UlyssesToken contract if assetBalance equals newAssetBalance, e.g. it will fall into the else branch:

function updateAssetBalances() internal {
    for (uint256 i = 0; i < assets.length; i++) {
        uint256 assetBalance = assets[i].balanceOf(address(this));
        uint256 newAssetBalance = totalSupply.mulDivUp(weights[i], totalWeights);

        if (assetBalance > newAssetBalance) {
            assets[i].safeTransfer(msg.sender, assetBalance - newAssetBalance);
        } else {
            assets[i].safeTransferFrom(msg.sender, address(this), newAssetBalance - assetBalance);
        }
    }
}

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

One issue that could happen is that some ERC20 tokens revert when transferring zero amount, the correct behavior would be for updateAssetBalances() to don't make any transfer if assetBalance equals newAssetBalance.

Impact

If such a token gets added in the vault and assetBalance equals newAssetBalance, e.g. the balanceOf the token in the contracts equal the (totalSupply * weight) / totalWeights, which could happen arbitrarily as, for example, when weights are updated, then calling addAsset() and removeAsset() will fail, since these functions call updateAssetBalances(). End result is DOS for adding/removing assets into UlyssesToken.

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

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

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

The amount of tokens with this behavior is small today, but it's possible that new tokens or existing tokens start to incorporate this behavior of reverting for zero amount. Since Maia is overall open, trustless and permissionless, it would be optimal to support tokens with this behavior.

Tools Used

Manual review.

Recommended Mitigation Steps

Prevent token transfers when assetBalance equals newAssetBalance in updateAssetBalances(), e.g:

git diff --no-index UlyssesToken.sol.orig UlyssesToken.sol
diff --git a/UlyssesToken.sol.orig b/UlyssesToken.sol
index c3c43f8..eabbdd2 100644
--- a/UlyssesToken.sol.orig
+++ b/UlyssesToken.sol
@@ -111,7 +111,9 @@ contract UlyssesToken is ERC4626MultiToken, Ownable, IUlyssesToken {
         for (uint256 i = 0; i < assets.length; i++) {
             uint256 assetBalance = assets[i].balanceOf(address(this));
             uint256 newAssetBalance = totalSupply.mulDivUp(weights[i], totalWeights);
-
+            if (assetBalance == newAssetBalance) {
+                continue;
+            }
             if (assetBalance > newAssetBalance) {
                 assets[i].safeTransfer(msg.sender, assetBalance - newAssetBalance);
             } else {

Alternatively:

git diff --no-index UlyssesToken.sol.orig UlyssesToken.sol
diff --git a/UlyssesToken.sol.orig b/UlyssesToken.sol
index c3c43f8..730d4f6 100644
--- a/UlyssesToken.sol.orig
+++ b/UlyssesToken.sol
@@ -111,10 +111,9 @@ contract UlyssesToken is ERC4626MultiToken, Ownable, IUlyssesToken {
         for (uint256 i = 0; i < assets.length; i++) {
             uint256 assetBalance = assets[i].balanceOf(address(this));
             uint256 newAssetBalance = totalSupply.mulDivUp(weights[i], totalWeights);
-
             if (assetBalance > newAssetBalance) {
                 assets[i].safeTransfer(msg.sender, assetBalance - newAssetBalance);
-            } else {
+            } else if (assetBalance < newAssetBalance) {
                 assets[i].safeTransferFrom(msg.sender, address(this), newAssetBalance - assetBalance);
             }
         }

Assessed type

Invalid Validation

c4-judge commented 1 year ago

trust1995 changed the severity to QA (Quality Assurance)