Maia DAO Ecosystem - 0xTheC0der's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1335-L1342 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L383-L390 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L269 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L313 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L696 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L745

Vulnerability details

Impact

First, the BranchBridgeAgent._normalizeDecimals(...) and BranchPort._denormalizeDecimals(...) methods, which are both part of the Ulysses Omnichain protocol, act exactly inverse to what they are supposed to do. For example, the _normalizeDecimals method should normalize any amount with given decimals to 18 decimals, e.g. calling with amount = 1e17 and decimals = 17 should return 1e18, but the actual result of the method is 1e16. The _denormalizeDecimals method should denormalize any amount with 18 decimals to given decimals, e.g. calling with amount = 1e18 and decimals = 17 should return 1e17, but the actual result of the method is 1e19.
Now this was only a mild example, considering there are legitimate tokens with only 6 decimals like USDT and USDC, the impacts of this bug can range from simple DoS to stuck funds or even loss of funds for user/protocol.

Second, the BranchBridgeAgent contract's methods callOutSignedAndBridge(...), callOutSignedAndBridgeMultiple(...), _callOutAndBridge(...) and _callOutAndBridgeMultiple fail to normalize the deposit token amounts before forwarding the call via _depositAndCall(...)/_depositAndCallMultiple(...) which subsequently calls ArbitrumBranchPort.bridgeOut(...)/ArbitrumBranchPort.bridgeOutMultiple(...). These last-mentioned methods denormalize the deposit token amounts and transfer the assets from the user. Therefore, it is crucial to normalize the token amounts before in order to avoid DoS, stuck funds or even loss of funds for user/protocol depending on given approvals and actual amounts.

Proof of Concept

The following PoC adds 2 new test cases to demonstrate the above issues in a simple omnichain deposit & redeem example using an underlying token with 17 (instead of 18) decimals. Both test cases will fail on token transfer from user due to incorrect/missing token amount normalization & denormalization.
Note that the test cases will pass and become more clear once the recommended mitigation steps below are applied.

Just apply the following diff and run all Ulysses Omnichain tests with forge test --match-path test/ulysses-omnichain/**:

diff --git a/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol b/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol
index 506b06d..cd41caf 100644
--- a/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol
+++ b/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol
@@ -164,6 +164,43 @@ contract BranchBridgeAgentTest is Test {
         );
     }

+    function testCallOutWithDepositD17() public returns (MockERC20, uint256, uint256) {
+        //Get some gas.
+        vm.deal(address(this), 1 ether);
+
+        MockERC20 underlyingToken2 = new MockERC20("underlying token2", "UNDER2", 17);
+        uint256 depositAmount = 100 * (10**underlyingToken2.decimals());   // 100 UNDER2 (with 17 decimals)
+        uint256 depositAmountNormalized = 100 ether;                       // 100 UNDER2 (normalized to 18 decimals)
+
+        //Mint Test tokens.
+        underlyingToken2.mint(address(this), depositAmount);
+
+        //Approve spend by router
+        underlyingToken2.approve(localPortAddress, depositAmount);
+
+        console2.log("Test CallOut Addresses:");
+        console2.log(address(testToken), address(underlyingToken2));
+
+        //Prepare deposit info
+        DepositInput memory depositInput = DepositInput({
+            hToken: address(testToken),
+            token: address(underlyingToken2),
+            amount: 100 ether,
+            deposit: depositAmount,
+            toChain: rootChainId
+        });
+
+        //Call Deposit function
+        IBranchRouter(bRouter).callOutAndBridge{value: 1 ether}(bytes("test"), depositInput, 0.5 ether);
+
+        //Test If Deposit was successful
+        testCreateDepositSingle(
+            uint32(1), address(this), address(testToken), address(underlyingToken2), 100 ether, depositAmountNormalized, 1 ether
+        );
+
+        return (underlyingToken2, depositAmount, depositAmountNormalized);
+    }
+
     function testCallOutInsufficientAmount() public {
         //Get some gas.
         vm.deal(address(this), 1 ether);
@@ -329,6 +366,65 @@ contract BranchBridgeAgentTest is Test {
         require(underlyingToken.balanceOf(localPortAddress) == 0);
     }

+    function testFallbackClearDepositRedeemSuccessD17() public {
+        vm.mockCall(
+            localAnyCallExecutorAddress,
+            abi.encodeWithSignature("context()"),
+            abi.encode(rootBridgeAgentAddress, rootChainId, 22)
+        );
+
+        // Create Test Deposit
+        (MockERC20 underlyingToken2, uint256 depositAmount, uint256 depositAmountNormalized) = testCallOutWithDepositD17();
+
+        vm.deal(localPortAddress, 1 ether);
+
+        //Prepare deposit info
+        DepositParams memory depositParams = DepositParams({
+            hToken: address(testToken),
+            token: address(underlyingToken2),
+            amount: 100 ether,
+            deposit: depositAmount,
+            toChain: rootChainId,
+            depositNonce: 1,
+            depositedGas: 1 ether
+        });
+
+        // Encode AnyFallback message
+        bytes memory anyFallbackData = abi.encodePacked(
+            bytes1(0x02),
+            depositParams.depositNonce,
+            depositParams.hToken,
+            depositParams.token,
+            depositParams.amount,
+            depositAmountNormalized,
+            depositParams.toChain,
+            bytes("testdata"),
+            depositParams.depositedGas,
+            depositParams.depositedGas / 2
+        );
+
+        vm.mockCall(
+            address(localAnyCongfig),
+            abi.encodeWithSignature(
+                "calcSrcFees(address,uint256,uint256)", address(0), rootChainId, anyFallbackData.length
+            ),
+            abi.encode(0)
+        );
+
+        // Call 'anyFallback'
+        vm.prank(localAnyCallExecutorAddress);
+        bAgent.anyFallback(anyFallbackData);
+
+        //Call redeemDeposit
+        bAgent.redeemDeposit(1);
+
+        // Check balances
+        require(testToken.balanceOf(address(this)) == 0);
+        require(underlyingToken2.balanceOf(address(this)) == depositAmount);
+        require(testToken.balanceOf(localPortAddress) == 0);
+        require(underlyingToken2.balanceOf(localPortAddress) == 0);
+    }
+
     function testFallbackClearDepositRedeemAlreadyRedeemed() public {
         vm.mockCall(
             localAnyCallExecutorAddress,
@@ -971,6 +1067,7 @@ contract BranchBridgeAgentTest is Test {
         console2.logUint(deposits[0]);

         if (hTokens[0] != address(0) || tokens[0] != address(0)) {
+            _deposit = (MockERC20(_token).decimals() == 18) ? _deposit : (_deposit * (10 ** MockERC20(_token).decimals())) / 1 ether; // convert normalized amount to actual amount
             if (amounts[0] > 0 && deposits[0] == 0) {
                 require(MockERC20(hTokens[0]).balanceOf(_user) == 0, "Deposit hToken balance doesn't match");

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

The diff below corrects the invalid normalization & denormalization methods as well as additionally normalizes token amounts where it was still missing (at the above mentioned instances).

diff --git a/src/ulysses-omnichain/BranchBridgeAgent.sol b/src/ulysses-omnichain/BranchBridgeAgent.sol
index 929c621..d227c07 100644
--- a/src/ulysses-omnichain/BranchBridgeAgent.sol
+++ b/src/ulysses-omnichain/BranchBridgeAgent.sol
@@ -241,6 +241,8 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
         lock
         requiresFallbackGas
     {
+        uint256 deposit = _normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals());
+
         //Encode Data for cross-chain call.
         bytes memory packedData = abi.encodePacked(
             bytes1(0x05),
@@ -249,7 +251,7 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
             _dParams.hToken,
             _dParams.token,
             _dParams.amount,
-            _normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals()),
+            deposit,
             _dParams.toChain,
             _params,
             msg.value.toUint128(),
@@ -266,7 +268,7 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
             _dParams.hToken,
             _dParams.token,
             _dParams.amount,
-            _dParams.deposit,
+            deposit,
             msg.value.toUint128()
         );
     }
@@ -310,7 +312,7 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
             _dParams.hTokens,
             _dParams.tokens,
             _dParams.amounts,
-            _dParams.deposits,
+            _deposits,
             msg.value.toUint128()
         );
     }
@@ -677,6 +679,8 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
         uint128 _gasToBridgeOut,
         uint128 _remoteExecutionGas
     ) internal {
+        uint256 deposit = _normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals());
+
         //Encode Data for cross-chain call.
         bytes memory packedData = abi.encodePacked(
             bytes1(0x02),
@@ -684,7 +688,7 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
             _dParams.hToken,
             _dParams.token,
             _dParams.amount,
-            _normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals()),
+            deposit,
             _dParams.toChain,
             _params,
             _gasToBridgeOut,
@@ -693,7 +697,7 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
 
         //Create Deposit and Send Cross-Chain request
         _depositAndCall(
-            _depositor, packedData, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit, _gasToBridgeOut
+            _depositor, packedData, _dParams.hToken, _dParams.token, _dParams.amount, deposit, _gasToBridgeOut
         );
     }
 
@@ -742,7 +746,7 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
             _dParams.hTokens,
             _dParams.tokens,
             _dParams.amounts,
-            _dParams.deposits,
+            deposits,
             _gasToBridgeOut
         );
     }
@@ -1338,7 +1342,7 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
      * @param _decimals number of decimal places
      */
     function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
-        return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
+        return _decimals == 18 ? _amount : (_amount * 1 ether) / (10 ** _decimals);
     }
 
     /**
diff --git a/src/ulysses-omnichain/BranchPort.sol b/src/ulysses-omnichain/BranchPort.sol
index b1bd210..a75b541 100644
--- a/src/ulysses-omnichain/BranchPort.sol
+++ b/src/ulysses-omnichain/BranchPort.sol
@@ -386,7 +386,7 @@ contract BranchPort is Ownable, IBranchPort {
      * @param _decimals number of decimal places
      */
     function _denormalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
-        return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
+        return _decimals == 18 ? _amount : (_amount * (10 ** _decimals)) / 1 ether;
     }
 
     /*///////////////////////////////////////////////////////////////

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

Asset IDs in the UlyssesToken contract are 1-based, see L49 in UlyssesToken.addAsset(...) and L55 in ERC4626MultiToken.constructor(...) of the parent contract.
However, when removing an asset from the UlyssesToken contract, the last added asset gets the 0-based ID of the removed asset, see L72 in UlyssesToken.removeAsset(...).

This leads to the following consequences:

1. Duplicate IDs when removing an asset.
   Example: We have assets with IDs 1,2,3,4. Next, asset with ID=2 is removed. Now we have assets with IDs 1,1,3 because the last asset with ID=4 gets ID=2-1=1.
2. Last asset cannot be removed after removing first asset.
   Example: Once the first asset with ID=1 is removed, the last asset gets ID=0 instead of ID=1. When trying to remove the last asset L62 in UlyssesToken.removeAsset(...) will revert due to underflow.
3. Last asset can be added a second time after removing first asset.
   Example: Once the first asset with ID=1 is removed, the last asset gets ID=0 instead of ID=1. When trying to add the last asset again L45 in UlyssesToken.addAsset(...) will not revert since ID=0 indicates that the asset wasn't added yet. Therefore, the underlying vault can contain the same token twice with different weights.

In conclusion, the asset accounting of the UlyssesToken contract is broken after removing an asset (especially the first one). This was also highlighted as a special area of concern in the audit details: ulysses AMM and token accounting

Proof of Concept

The above issues are demostrated by the new test cases test_UlyssesTokenAddAssetTwice and test_UlyssesTokenRemoveAssetFail, just apply the diff below and run the tests with forge test --match-test test_UlyssesToken.

diff --git a/test/ulysses-amm/UlyssesTokenTest.t.sol b/test/ulysses-amm/UlyssesTokenTest.t.sol
index bdb4a7d..dcf6d45 100644
--- a/test/ulysses-amm/UlyssesTokenTest.t.sol
+++ b/test/ulysses-amm/UlyssesTokenTest.t.sol
@@ -3,6 +3,7 @@ pragma solidity >=0.8.0 <0.9.0;

 import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
 import {UlyssesToken} from "@ulysses-amm/UlyssesToken.sol";
+import {IUlyssesToken} from "@ulysses-amm/interfaces/IUlyssesToken.sol";

 import {UlyssesTokenHandler} from "@test/test-utils/invariant/handlers/UlyssesTokenHandler.t.sol";

@@ -29,4 +30,28 @@ contract InvariantUlyssesToken is UlyssesTokenHandler {
         _vaultMayBeEmpty = true;
         _unlimitedAmount = false;
     }
+
+    function test_UlyssesTokenRemoveAssetFail() public  {
+        UlyssesToken token = UlyssesToken(_vault_);
+
+        // remove first asset with ID=1
+        token.removeAsset(_underlyings_[0]);
+        // due to accounting error, last asset now has ID=0 instead of ID=1
+
+        // remove last asset --> underflow error due to ID=0
+        token.removeAsset(_underlyings_[NUM_ASSETS - 1]);
+    }
+
+    function test_UlyssesTokenAddAssetTwice() public  {
+        UlyssesToken token = UlyssesToken(_vault_);
+
+        // remove first asset with ID=1
+        token.removeAsset(_underlyings_[0]);
+        // due to accounting error, last asset now has ID=0 instead of ID=1
+
+        // add last asset again --> doesn't revert since it "officially" doesn't exist due to ID=1
+        vm.expectRevert(IUlyssesToken.AssetAlreadyAdded.selector);
+        token.addAsset(_underlyings_[NUM_ASSETS - 1], 1);
+    }
+
 }

We can see that adding the last asset again does not revert but trying to remove it still fails:

Encountered 2 failing tests in test/ulysses-amm/UlyssesTokenTest.t.sol:InvariantUlyssesToken
[FAIL. Reason: Call did not revert as expected] test_UlyssesTokenAddAssetTwice() (gas: 169088)
[FAIL. Reason: Arithmetic over/underflow] test_UlyssesTokenRemoveAssetFail() (gas: 137184)

Tools Used

VS Code, Foundry, MS Excel

Recommended Mitigation Steps

Fortunately, all of the above issues can be easily fixed by using an 1-based asset ID in L72 of UlyssesToken.removeAsset(...):

diff --git a/src/ulysses-amm/UlyssesToken.sol b/src/ulysses-amm/UlyssesToken.sol
index 552a125..0937e9f 100644
--- a/src/ulysses-amm/UlyssesToken.sol
+++ b/src/ulysses-amm/UlyssesToken.sol
@@ -69,7 +69,7 @@ contract UlyssesToken is ERC4626MultiToken, Ownable, IUlyssesToken {

         address lastAsset = assets[newAssetsLength];

-        assetId[lastAsset] = assetIndex;
+        assetId[lastAsset] = assetIndex + 1;
         assets[assetIndex] = lastAsset;
         weights[assetIndex] = weights[newAssetsLength];

After applying the recommended fix, both new test cases pass:

[PASS] test_UlyssesTokenAddAssetTwice() (gas: 122911)
[PASS] test_UlyssesTokenRemoveAssetFail() (gas: 134916)

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L243-L268

Vulnerability details

Impact

Since RootBridgeAgent.retrySettlement(...) can be called by anyone for any settlement, a malicious actor can front-run an user trying to redeem his failed settlement via RootBridgeAgent.redeemSettlement(...) by calling RootBridgeAgent.retrySettlement(...) with _remoteExecutionGas = 0 in order to make sure that this settlement will also fail in the future.

As a consequnce, the user's subsequent call to RootBridgeAgent.redeemSettlement(...) will revert (DoS) because the settlement was already marked with SettlementStatus.Success during the malicious actor's call to RootBridgeAgent.retrySettlement(...). Therefore the user is unable to redeem his assets.

Proof of Concept

The following PoC modifies an existing test case to confirm the above claims resulting in:

• The settlement being marked with SettlementStatus.Success.
• DoS of RootBridgeAgent.redeemSettlement(...) method for this settlement.
• The user not being able to redeem his assets.

Just apply the diff below and run the test with forge test --match-test testRedeemSettlement:

diff --git a/test/ulysses-omnichain/RootTest.t.sol b/test/ulysses-omnichain/RootTest.t.sol
index ea88453..ccd7ad2 100644
--- a/test/ulysses-omnichain/RootTest.t.sol
+++ b/test/ulysses-omnichain/RootTest.t.sol
@@ -1299,14 +1299,13 @@ contract RootTest is DSTestPlus {
         hevm.deal(_user, 1 ether);
 
         //Retry Settlement
-        multicallBridgeAgent.retrySettlement{value: 1 ether}(settlementNonce, 0.5 ether);
 
         settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);
 
         require(settlement.status == SettlementStatus.Success, "Settlement status should be success.");
     }
 
-    function testRedeemSettlement() public {
+    function testRedeemSettlementFrontRunDoS() public {
         //Set up
         testAddLocalTokenArbitrum();
 
@@ -1389,15 +1388,25 @@ contract RootTest is DSTestPlus {
 
         require(settlement.status == SettlementStatus.Failed, "Settlement status should be failed.");
 
-        //Retry Settlement
-        multicallBridgeAgent.redeemSettlement(settlementNonce);
+        //Front-run redeem settlement with '_remoteExecutionGas = 0'
+        address _malice = address(0x1234);
+        hevm.deal(_malice, 1 ether);
+        hevm.prank(_malice);
+        multicallBridgeAgent.retrySettlement{value: 1 ether}(settlementNonce, 0 ether);
 
         settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);
+        require(settlement.status == SettlementStatus.Success, "Settlement status should be success.");
 
-        require(settlement.owner == address(0), "Settlement should cease to exist.");
+        //Redeem settlement DoS cause settlement is marked as success
+        hevm.expectRevert(abi.encodeWithSignature("SettlementRedeemUnavailable()"));
+        multicallBridgeAgent.redeemSettlement(settlementNonce);
+
+        settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);
+        require(settlement.owner != address(0), "Settlement should still exist.");
 
+        //User couldn't redeem funds
         require(
-            MockERC20(newAvaxAssetGlobalAddress).balanceOf(_user) == 150 ether, "Settlement should have been redeemed"
+            MockERC20(newAvaxAssetGlobalAddress).balanceOf(_user) == 0 ether, "Settlement should not have been redeemed"
         );
     }
 

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

I suggest to only allow calls to RootBridgeAgent.retrySettlement(...) by the settlement owner:

diff --git a/src/ulysses-omnichain/RootBridgeAgent.sol b/src/ulysses-omnichain/RootBridgeAgent.sol
index 34f4286..4acef39 100644
--- a/src/ulysses-omnichain/RootBridgeAgent.sol
+++ b/src/ulysses-omnichain/RootBridgeAgent.sol
@@ -242,6 +242,14 @@ contract RootBridgeAgent is IRootBridgeAgent {
 
     /// @inheritdoc IRootBridgeAgent
     function retrySettlement(uint32 _settlementNonce, uint128 _remoteExecutionGas) external payable {
+        //Get deposit owner.
+        address depositOwner = getSettlement[_settlementNonce].owner;
+        if (
+            msg.sender != depositOwner && msg.sender != address(IPort(localPortAddress).getUserAccount(depositOwner))
+        ) {
+            revert NotSettlementOwner();
+        }
+
         //Update User Gas available.
         if (initialGas == 0) {
             userFeeInfo.depositedGas = uint128(msg.value);

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/tokens/ERC4626PartnerManager.sol#L235-L246 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/tokens/ERC4626PartnerManager.sol#L215-L229

Vulnerability details

Impact

Once a user deposits Maia ERC-20 tokens into the vMaia ERC-4626 vault, he is eligible to claim 3 kinds of utility tokens, bHermes Weight & Governance and Maia Governance (pbHermes, partner governance). On each deposit, new Maia Governance tokens (pbHermes) are minted to the vault in proportion to the deposited amount, but those tokens are never burned on withdrawal. This naturally dilutes the vault's pbHermes token balance during the course of users depositing & withdrawing Maia tokens. Futhermore, a malicious user can dramatically accelerate this dilution by repeatedly depositing & withdrawing within a single transaction.
Note that the vault's bHermes Weight & Governance token balances are not diluted during this process.

However, the ERC4626PartnerManager.increaseConversionRate(...) method (ERC4626PartnerManager is base of vMaia contract) relies on the vault's pbHermes token balance and therefore imposes a lower limt on an increased pbHermes<>bHermes coversion rate to avoid underflow, see L226: min. rate = vault balance of pbHermes / Maia tokens in vault. Meanwhile the upper limit for a new conversion rate is given by L219: max. rate = vault balance of bHermes / Maia tokens in vault.

As a consquence, the vMaia vault owner's ability to increase the conversion rate is successively constrained by user deposits & withdrawals, until the point where the dilution of pbHermes reaches vault balance of pbHermes > vault balance of bHermes which leads to complete DoS of the ERC4626PartnerManager.increaseConversionRate(...) method.

Proof of Concept

The following PoC verifies the above claims about pbHermes dilution and increaseConversionRate(...) DoS, just apply the diff below and run the new in-line documented test case with forge test -vv --match-test testDepositMaiaDilutionUntilConversionRateFailure:

diff --git a/test/maia/vMaiaTest.t.sol b/test/maia/vMaiaTest.t.sol
index 6efabc5..2af982e 100644
--- a/test/maia/vMaiaTest.t.sol
+++ b/test/maia/vMaiaTest.t.sol
@@ -7,6 +7,7 @@ import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";

 import {vMaia, PartnerManagerFactory, ERC20} from "@maia/vMaia.sol";
 import {IBaseVault} from "@maia/interfaces/IBaseVault.sol";
+import {IERC4626PartnerManager} from "@maia/interfaces/IERC4626PartnerManager.sol";
 import {MockVault} from "./mock/MockVault.t.sol";

 import {bHermes} from "@hermes/bHermes.sol";
@@ -47,7 +48,7 @@ contract vMaiaTest is DSTestPlus {
             "vMAIA",
             address(bhermes),
             address(vault),
-            address(0)
+            address(this) // set owner to allow call to 'increaseConversionRate'
         );
     }

@@ -86,6 +87,39 @@ contract vMaiaTest is DSTestPlus {
         assertEq(vmaia.balanceOf(address(this)), amount);
     }

+    function testDepositMaiaDilutionUntilConversionRateFailure() public {
+        testDepositMaia();
+        uint256 amount = vmaia.balanceOf(address(this));
+
+        // fast-forward to withdrawal Tuesday
+        hevm.warp(getFirstDayOfNextMonthUnix());
+
+        for(uint256 i = 0; i < 10; i++) {
+            // get & print bHermes & pbHermes vault balances
+            uint256 bHermesBal = bhermes.balanceOf(address(vmaia));
+            uint256 pbHermesBal = vmaia.partnerGovernance().balanceOf(address(vmaia));
+            console2.log("vault balance of bHermes: ", bHermesBal);
+            console2.log("vault balance of pbHermes:", pbHermesBal);
+
+            // dilute pbHermes by withdraw & deposit cycle
+            vmaia.withdraw(amount, address(this), address(this));
+            maia.approve(address(vmaia), amount);
+            vmaia.deposit(amount, address(this));
+
+            // get diluted pbHermes balance and compute min. conversion rate accordingly
+            pbHermesBal = vmaia.partnerGovernance().balanceOf(address(vmaia));
+            uint256 minNewConversionRate = pbHermesBal / vmaia.totalSupply();
+            // check if dilution caused constraints are so bad that we get DoS
+            if (pbHermesBal > bHermesBal)
+            {
+                hevm.expectRevert(IERC4626PartnerManager.InsufficientBacking.selector);
+            }
+            vmaia.increaseConversionRate(minNewConversionRate);
+        }
+
+
+    }
+
     function testDepositMaiaAmountFail() public {
         assertEq(vmaia.bHermesRate(), bHermesRate);

We can clearly see the increasing dilution after each withdrawal-deposit cycle and get the expected revert, see if-condition, after reaching critical dilution:

[PASS] testDepositMaiaDilutionUntilConversionRateFailure() (gas: 1759462)
Logs:
  2023 2
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 200000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 400000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 800000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 1600000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 2400000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 3200000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 4000000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 4800000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 5600000000000000000000

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Burn the excess pbHermes tokens on withdrawal from vMaia vault:

diff --git a/src/maia/tokens/ERC4626PartnerManager.sol b/src/maia/tokens/ERC4626PartnerManager.sol
index b912bab..31cfef7 100644
--- a/src/maia/tokens/ERC4626PartnerManager.sol
+++ b/src/maia/tokens/ERC4626PartnerManager.sol
@@ -252,6 +252,7 @@ abstract contract ERC4626PartnerManager is PartnerUtilityManager, Ownable, ERC46
      * @param amount amounts of vMaia to burn
      */
     function _burn(address from, uint256 amount) internal virtual override checkTransfer(from, amount) {
+        ERC20MultiVotes(partnerGovernance).burn(address(this), amount * bHermesRate);
         super._burn(from, amount);
     }

We can see that this fixes the dilution issue:

[PASS] testDepositMaiaDilutionUntilConversionRateFailure() (gas: 2150656)
Logs:
  2023 2
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000
  vault balance of bHermes:  1000000000000000000000
  vault balance of pbHermes: 100000000000000000000

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/tokens/ERC4626PartnerManager.sol#L215-L229 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/vMaia.sol#L66-L88

Vulnerability details

Impact

Once a user deposits Maia ERC-20 tokens into the vMaia ERC-4626 vault, he is eligible to claim 3 kinds of utility tokens, bHermes Weight & Governance and Maia Governance (pbHermes, partner governance), via the ERC4626PartnerManager.claimOutstanding() method (ERC4626PartnerManager is base of vMaia contract).
The conversion rate between the utility tokens and vMaia tokens minted on deposit can be increased (and only increased) by the contract owner via the ERC4626PartnerManager.increaseConversionRate(...) method.

However, the checkWeight, checkGovernance & checkPartnerGovernance modifiers in the vMaia contract do not account for this conversion rate and therefore implicity only allow a conversion rate of 1.

As a consequence, as soon as the conversion rate is increased to > 1, a call to ERC4626PartnerManager.claimOutstanding() will inevitably revert due to subsequent calls to the above modifiers. Since the conversion rate can only be increased and the vMaia vault contract is not upgradeable, the claimOutstanding() method is subject to permanent DoS.
Of course, the user can still claim a reduced amount of utility tokens (according to a conversion rate of 1) via the PartnerUtilityManager.claimMultipleAmounts(...) method (PartnerUtilityManager is base of ERC4626PartnerManager contract), but this still implies a loss of assets for the user since not all utility tokens he is eligible for can be claimed. Furthermore, this workaround doesn't help when the user is a contract which implemented a call to the claimOutstanding() method.

Proof of Concept

The following PoC demonstrates the above DoS when trying to claim the utility tokens with increased conversion rate, just apply the diff below and run the test cases with forge test -vv --match-test testDepositMaia:

diff --git a/test/maia/vMaiaTest.t.sol b/test/maia/vMaiaTest.t.sol
index 6efabc5..499abb6 100644
--- a/test/maia/vMaiaTest.t.sol
+++ b/test/maia/vMaiaTest.t.sol
@@ -7,9 +7,11 @@ import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";

 import {vMaia, PartnerManagerFactory, ERC20} from "@maia/vMaia.sol";
 import {IBaseVault} from "@maia/interfaces/IBaseVault.sol";
+import {IERC4626PartnerManager} from "@maia/interfaces/IERC4626PartnerManager.sol";
 import {MockVault} from "./mock/MockVault.t.sol";

 import {bHermes} from "@hermes/bHermes.sol";
+import {IUtilityManager} from "@hermes/interfaces/IUtilityManager.sol";

 import {DateTimeLib} from "solady/utils/DateTimeLib.sol";

@@ -47,7 +49,7 @@ contract vMaiaTest is DSTestPlus {
             "vMAIA",
             address(bhermes),
             address(vault),
-            address(0)
+            address(this) // set owner to allow call to 'increaseConversionRate'
         );
     }

@@ -86,6 +88,33 @@ contract vMaiaTest is DSTestPlus {
         assertEq(vmaia.balanceOf(address(this)), amount);
     }

+    function testDepositMaiaClaimDoS() public {
+        testDepositMaia();
+
+        // increase 'pbHermes<>bHermes' conversion rate
+        vmaia.increaseConversionRate(bHermesRate * 2);
+
+        // claim utility tokens DoS
+        hevm.expectRevert(IUtilityManager.InsufficientShares.selector);
+        vmaia.claimOutstanding();
+
+        // cannot undo conversion rate -> claimOutstanding() method is broken forever
+        hevm.expectRevert(IERC4626PartnerManager.InvalidRate.selector);
+        vmaia.increaseConversionRate(bHermesRate);
+    }
+
+    function testDepositMaiaClaimSuccess() public {
+        testDepositMaia();
+
+        vmaia.claimOutstanding();
+
+        // got utility tokens as expected
+        assertGt(vmaia.bHermesToken().gaugeWeight().balanceOf(address(this)), 0);
+        assertGt(vmaia.bHermesToken().governance().balanceOf(address(this)), 0);
+        assertGt(vmaia.partnerGovernance().balanceOf(address(this)), 0);
+    }
+
+
     function testDepositMaiaAmountFail() public {
         assertEq(vmaia.bHermesRate(), bHermesRate);

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Simply remove the incorrect checkWeight, checkGovernance & checkPartnerGovernance modifiers from the vMaia contract, since the correct modifiers, which account for the conversion rate, are already implemented in the ERC4626PartnerManager contract.

diff --git a/src/maia/vMaia.sol b/src/maia/vMaia.sol
index 3aa70cf..5ee6f66 100644
--- a/src/maia/vMaia.sol
+++ b/src/maia/vMaia.sol
@@ -59,34 +59,6 @@ contract vMaia is ERC4626PartnerManager {
         currentMonth = DateTimeLib.getMonth(block.timestamp);
     }

-    /*///////////////////////////////////////////////////////////////
-                            MODIFIERS
-    //////////////////////////////////////////////////////////////*/
-
-    /// @dev Checks available weight allows for the call.
-    modifier checkWeight(uint256 amount) virtual override {
-        if (balanceOf[msg.sender] < amount + userClaimedWeight[msg.sender]) {
-            revert InsufficientShares();
-        }
-        _;
-    }
-
-    /// @dev Checks available governance allows for the call.
-    modifier checkGovernance(uint256 amount) virtual override {
-        if (balanceOf[msg.sender] < amount + userClaimedGovernance[msg.sender]) {
-            revert InsufficientShares();
-        }
-        _;
-    }
-
-    /// @dev Checks available partner governance allows for the call.
-    modifier checkPartnerGovernance(uint256 amount) virtual override {
-        if (balanceOf[msg.sender] < amount + userClaimedPartnerGovernance[msg.sender]) {
-            revert InsufficientShares();
-        }
-        _;
-    }
-
     /// @dev Boost can't be claimed; does not fail. It is all used by the partner vault.
     function claimBoost(uint256 amount) public override {}

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/vMaia.sol#L109-L110

Vulnerability details

Impact

The withdrawal of Maia ERC-20 tokens from the vMaia ERC-4626 vault should only be possible on the first Tuesday of the month.
In fact, the withdrawal is possible on the first Tuesday of attempted withdrawal of the month, due to the insufficient check in L109/110 of vMaia.beforeWithdraw(...) which only requires the current day to be a Tuesday.
Example: In case no Maia DAO member withdraws his Maia tokens on the first Tuesday of the month, withdrawal on the second Tuesday of the month and so forth is still possible.

It is stated at multiple instances throughout the ecosystem that the withdrawal is only possible on the first Tuesday of the month:

• In code: L23/24 and L100 of vMaia.sol
• In readme: L200 of README.md
• In documentation: vMaia tokenomics (mistake: Monday instead of Tuesday is written there as of 2023-06-30)

Therefore a base assumption for DAO members about the Maia ecosystem is broken. Potential withdrawals outside the specified time frame might cause problems for the front-end as well as lead to an unfair advantage for users who know about this loophole in contrast to those who don't.

Proof of Concept

The following test case demonstrates that a withdrawal is also possible on the second Tuesday of the month in case there was no withdrawal on the first. Just add this test case to test/maia/vMaiaTest.t.sol:vMaiaTest and run it with forge test -vv --match-test testWithdrawMaiaOnSecondTuesday:

function testWithdrawMaiaOnSecondTuesday() public {
    testDepositMaia();

    uint256 amount = 100 ether;

    hevm.warp(getFirstDayOfNextMonthUnix() + 7 days); // add 7 days -> second Tuesday

    vmaia.withdraw(amount, address(this), address(this));

    assertEq(maia.balanceOf(address(vmaia)), 0);
    assertEq(vmaia.balanceOf(address(this)), 0);
}

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

The issue can be resolved by strictly enforcing the first Tuesday of the month:

diff --git a/src/maia/libraries/DateTimeLib.sol b/src/maia/libraries/DateTimeLib.sol
index 29e6910..751f754 100644
--- a/src/maia/libraries/DateTimeLib.sol
+++ b/src/maia/libraries/DateTimeLib.sol
@@ -52,11 +52,12 @@ library DateTimeLib {

     /// @dev Returns the weekday from the unix timestamp.
     /// Monday: 1, Tuesday: 2, ....., Sunday: 7.
-    function isTuesday(uint256 timestamp) internal pure returns (bool result, uint256 startOfDay) {
+    function isFirstTuesdayOfMonth(uint256 timestamp) internal pure returns (bool result, uint256 startOfDay) {
         unchecked {
             uint256 day = timestamp / 86400;
             startOfDay = day * 86400;
-            result = ((day + 3) % 7) + 1 == 2;
+            // check if day is a Tuesday and previous Tuesday was previous month
+            result = ((day + 3) % 7) + 1 == 2 && getMonth(timestamp) == getMonth(timestamp - 7 * 86400) + 1;
         }
     }
 }
diff --git a/src/maia/vMaia.sol b/src/maia/vMaia.sol
index 3aa70cf..592a9ed 100644
--- a/src/maia/vMaia.sol
+++ b/src/maia/vMaia.sol
@@ -106,7 +106,7 @@ contract vMaia is ERC4626PartnerManager {
         uint256 _currentMonth = DateTimeLib.getMonth(block.timestamp);
         if (_currentMonth == currentMonth) revert UnstakePeriodNotLive();

-        (bool isTuesday, uint256 _unstakePeriodStart) = DateTimeLib.isTuesday(block.timestamp);
+        (bool isTuesday, uint256 _unstakePeriodStart) = DateTimeLib.isFirstTuesdayOfMonth(block.timestamp);
         if (!isTuesday) revert UnstakePeriodNotLive();

         currentMonth = _currentMonth;

Assessed type

Invalid Validation

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

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