Maia DAO Ecosystem - RED-LOTUS-REACH's results

Judge has assessed an item in Issue #835 as 2 risk. The relevant finding follows:

L-02

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340-L1342 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L388-L390

Vulnerability details

Impact:

• A critical business logic and accounting error has been discovered in the Ulysses Omnichain Protocol. This error will prevent users from depositing, withdrawing, or swapping tokens with decimal values not equal to 18.
• The affected code has been identified in the following contracts: BranchBridgeAgent.sol, ArbitrumBridgeAgent.sol, RootBridgeAgent.sol, BranchPort.sol, and ArbitrumBranchPort.sol.
• The errors can be categorized as follows:

Accounting Error

• The issue arises from the use of the 'ether' denomination in the calculation.
• Specifically, the _amount * 1 ether / (10 ** _decimals) formula in _denormalizeDecimals and _amount * (10 ** _decimals) / 1 ether formula in _normalizeDecimals.
• These calculations are flawed because 1 ether is equal to 10 ** 18, which leads to incorrect results when dealing with tokens such as USDC, USDT (6 decimals), or GUSDC (2 decimals).
• This error makes it impossible for users to deposit funds on either the Arbitrum Branch or a generic Branch chain, as demonstrated in the proof of concept section.

Business Logic Error

• Even if the normalizeDecimals and denormalizeDecimals functions are implemented correctly, a critical business logic error arises.
• Inconsistent or unimplemented passing of normalizeDecimals and denormlizeDecimals in secondary code prevents the proper utilization of these functions.
• As a result, users are unable to withdraw tokens or swap tokens between chains.
• This error significantly impacts the functionality of the Ulysses Omnichain protocol.

Wider Perspective / Analysis

• Considering that widely-used stablecoins like USDC and USDT, along with other tokens, commonly have decimal values less than 18, the impact of this error cannot be underestimated.
• The proof of concept below provides a comprehensive demonstration of the affected functions and their failure.
• To fully address this issue, extensive code refactoring is necessary across multiple contracts, and we will provide detailed mitigation steps for resolving the problem.

Proof of Concept:

• The vulnerability lies in:
  • BranchBridgeAgent.sol::_normalizedecimals()
  • BranchPort.sol::_denormalizedecimals()
• Here are the relevant code snippets:

function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
        return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
    }

function _denormalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
        return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
    }

• In order to demonstrate the vulnerability, it is necessary to change the decimal value of the underlying token to 6 in the following locations:
  • ArbitrumBranchTest.t.sol: Line 412
  • BranchBridgeAgentTest.t.sol: Line 55
• After making the above changes, executing the following commands will result in complete failure, as the protocol cannot handle the normalized decimal values:

~/2023-05-maia$ forge test --match-contract ArbitrumBranchTest

Encountered 4 failing tests in test/ulysses-omnichain/ArbitrumBranchTest.t.sol:ArbitrumBranchTest
[FAIL. Reason: TransferFromFailed()] testCallOutWithDeposit() (gas: 1334098)
[FAIL. Reason: User should have 0 tokens] testDepositToPort() (gas: 1385596)
[FAIL. Reason: User should have 0 tokens] testWithdrawFromPort() (gas: 1385568)
[FAIL. Reason: User should have 0 tokens] testWithdrawFromPortUnknown() (gas: 1385545)

~/2023-05-maia$ forge test --match-contract BranchBridgeAgentTest

Encountered 7 failing tests in test/ulysses-omnichain/BranchBridgeAgentTest.t.sol:BranchBridgeAgentTest
[FAIL. Reason: TransferFromFailed()] testCallOutWithDeposit() (gas: 175740)
[FAIL. Reason: TransferFromFailed()] testFallbackClearDepositRedeemAlreadyRedeemed() (gas: 181567)
[FAIL. Reason: TransferFromFailed()] testFallbackClearDepositRedeemDoubleAnycall() (gas: 181502)
[FAIL. Reason: TransferFromFailed()] testFallbackClearDepositRedeemSuccess() (gas: 181546)
[FAIL. Reason: TransferFromFailed()] testRetryDeposit() (gas: 181567)
[FAIL. Reason: TransferFromFailed()] testRetryDepositFailCanAlwaysRetry() (gas: 181521)
[FAIL. Reason: TransferFromFailed()] testRetryDepositFailNotOwner() (gas: 181544)

Tools Used:

Manual Review and Foundry

Recommended Mitigation Steps

• It is recommended to use normalized value within the omnichain and when the values are passed to external contracts, they should be denormalized.
• Furthermore, the provided diff only addresses the issues that arise when the tests fail. It does not address the functions that are not covered in the test file.
• The accounting error in BranchBridgeAgent.sol::_normalizedecimals() and BranchPort.sol::_denormalizedecimals() has been resolved with the following git diff.
• Additionally, the logic error related to the inconsistent use of the aforementioned functions has also been addressed.
• Steps:
  • Ensure the repository is checked out to commit: 54a45be
  • Copy the following diff to decimals.patch and save at the top level of the 2023-05-maia/ repository.
  • Run the command git apply decimals.patch while at the top level of the 2023-05-maia/ repository.
  • Confirm that all related tests pass using the commands
    • forge test --match-contract ArbitrumBranchTest
    • forge test --match-contract BranchBridgeAgentTest

Link to Gist for decimals.patch

https://gist.github.com/0xreentrant/303948546ff1d5bda080cdb43a8938f5

diff --git a/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol b/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol
index 6620591..f11b9dd 100644
--- a/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol
+++ b/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol
@@ -109,13 +109,14 @@ contract ArbitrumBranchBridgeAgent is BranchBridgeAgent {
      */
     function withdrawFromPort(
         address localAddress,
+        address underlyingAddress,
         uint256 amount
     ) external payable lock {
         IArbPort(localPortAddress).withdrawFromPort(
             msg.sender,
             msg.sender,
             localAddress,
-            amount
+            _normalizeDecimals(amount, ERC20(underlyingAddress).decimals())
         );
     }
 
diff --git a/src/ulysses-omnichain/ArbitrumBranchPort.sol b/src/ulysses-omnichain/ArbitrumBranchPort.sol
index 2f5ff33..b2b3e0d 100644
--- a/src/ulysses-omnichain/ArbitrumBranchPort.sol
+++ b/src/ulysses-omnichain/ArbitrumBranchPort.sol
@@ -64,7 +64,7 @@ contract ArbitrumBranchPort is BranchPort, IArbitrumBranchPort {
         _underlyingAddress.safeTransferFrom(
             _depositor,
             address(this),
-            _deposit
+            _denormalizeDecimals(_deposit, ERC20(_underlyingAddress).decimals())
         );
 
         IRootPort(rootPortAddress).mintToLocalBranch(
@@ -113,10 +113,7 @@ contract ArbitrumBranchPort is BranchPort, IArbitrumBranchPort {
         address _underlyingAddress,
         uint256 _deposit
     ) external override(IBranchPort, BranchPort) requiresBridgeAgent {
-        _underlyingAddress.safeTransfer(
-            _recipient,
-            _denormalizeDecimals(_deposit, ERC20(_underlyingAddress).decimals())
-        );
+        _underlyingAddress.safeTransfer(_recipient, _deposit);
     }
 
     /// @inheritdoc IBranchPort
@@ -169,11 +166,16 @@ contract ArbitrumBranchPort is BranchPort, IArbitrumBranchPort {
                 )
             );
         }
-        if (_amount - _deposit > 0) {
+        uint x = _amount -
+            _denormalizeDecimals(
+                _deposit,
+                ERC20(_underlyingAddress).decimals()
+            );
+        if (x > 0) {
             IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(
                 _depositor,
                 _localAddress,
-                _amount - _deposit
+                x
             );
         }
     }
diff --git a/src/ulysses-omnichain/BranchBridgeAgent.sol b/src/ulysses-omnichain/BranchBridgeAgent.sol
index 6ddbe62..1fcc4ae 100644
--- a/src/ulysses-omnichain/BranchBridgeAgent.sol
+++ b/src/ulysses-omnichain/BranchBridgeAgent.sol
@@ -299,7 +299,10 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
             _dParams.hToken,
             _dParams.token,
             _dParams.amount,
-            _dParams.deposit,
+            _normalizeDecimals(
+                _dParams.deposit,
+                ERC20(_dParams.token).decimals()
+            ),
             msg.value.toUint128()
         );
     }
@@ -823,7 +826,10 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
             _dParams.hToken,
             _dParams.token,
             _dParams.amount,
-            _dParams.deposit,
+            _normalizeDecimals(
+                _dParams.deposit,
+                ERC20(_dParams.token).decimals()
+            ),
             _gasToBridgeOut
         );
     }
@@ -1038,7 +1044,7 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
         uint256[] memory amounts = new uint256[](1);
         amounts[0] = _amount;
         uint256[] memory deposits = new uint256[](1);
-        deposits[0] = _deposit;
+        deposits[0] = _denormalizeDecimals(_deposit, ERC20(_token).decimals());
 
         // Update State
         getDeposit[_getAndIncrementDepositNonce()] = Deposit({
@@ -1125,7 +1131,10 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
                 deposit.hTokens[i],
                 deposit.tokens[i],
                 deposit.amounts[i],
-                deposit.deposits[i]
+                _normalizeDecimals(
+                    deposit.deposits[i],
+                    ERC20(deposit.tokens[i]).decimals()
+                )
             );
 
             unchecked {
@@ -1157,7 +1166,9 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
         uint256 _amount,
         uint256 _deposit
     ) internal {
-        if (_amount - _deposit > 0) {
+        uint256 x = _amount -
+            _denormalizeDecimals(_deposit, ERC20(_token).decimals());
+        if (x > 0) {
             IPort(localPortAddress).bridgeIn(
                 _recipient,
                 _hToken,
@@ -1589,8 +1600,11 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
         uint256 _amount,
         uint8 _decimals
     ) internal pure returns (uint256) {
-        return
-            _decimals == 18 ? _amount : (_amount * (10 ** _decimals)) / 1 ether;
+        if (_decimals > 18) {
+            return _amount / (10 ** (_decimals - 18));
+        } else {
+            return _amount * (10 ** (18 - _decimals));
+        }
     }
 
     /**
@@ -1610,6 +1624,17 @@ contract BranchBridgeAgent is IBranchBridgeAgent {
         }
     }
 
+    function _denormalizeDecimals(
+        uint256 _amount,
+        uint8 _decimals
+    ) internal pure returns (uint256) {
+        if (_decimals > 18) {
+            return _amount * (10 ** (_decimals - 18));
+        } else {
+            return _amount / (10 ** (18 - _decimals));
+        }
+    }
+
     /*///////////////////////////////////////////////////////////////
                                 MODIFIERS
     //////////////////////////////////////////////////////////////*/
diff --git a/src/ulysses-omnichain/BranchPort.sol b/src/ulysses-omnichain/BranchPort.sol
index e40c9a4..730b108 100644
--- a/src/ulysses-omnichain/BranchPort.sol
+++ b/src/ulysses-omnichain/BranchPort.sol
@@ -285,13 +285,15 @@ contract BranchPort is Ownable, IBranchPort {
         uint256 _amount,
         uint256 _deposit
     ) external virtual requiresBridgeAgent {
-        if (_amount - _deposit > 0) {
-            _localAddress.safeTransferFrom(
-                _depositor,
-                address(this),
-                _amount - _deposit
+        //@audit-info C3
+        uint256 x = _amount -
+            _denormalizeDecimals(
+                _deposit,
+                ERC20(_underlyingAddress).decimals()
             );
-            ERC20hTokenBranch(_localAddress).burn(_amount - _deposit);
+        if (x > 0) {
+            _localAddress.safeTransferFrom(_depositor, address(this), x);
+            ERC20hTokenBranch(_localAddress).burn(x);
         }
         if (_deposit > 0) {
             _underlyingAddress.safeTransferFrom(
@@ -471,8 +473,11 @@ contract BranchPort is Ownable, IBranchPort {
         uint256 _amount,
         uint8 _decimals
     ) internal pure returns (uint256) {
-        return
-            _decimals == 18 ? _amount : (_amount * 1 ether) / (10 ** _decimals);
+        if (_decimals > 18) {
+            return _amount * (10 ** (_decimals - 18));
+        } else {
+            return _amount / (10 ** (18 - _decimals));
+        }
     }
 
     /*///////////////////////////////////////////////////////////////
diff --git a/src/ulysses-omnichain/RootBridgeAgent.sol b/src/ulysses-omnichain/RootBridgeAgent.sol
index 93f64bb..02ffd94 100644
--- a/src/ulysses-omnichain/RootBridgeAgent.sol
+++ b/src/ulysses-omnichain/RootBridgeAgent.sol
@@ -43,7 +43,11 @@ library CheckParamsLib {
         uint24 _fromChain
     ) internal view returns (bool) {
         if (
-            (_dParams.amount < _dParams.deposit) || //Deposit can't be greater than amount.
+            (_dParams.amount <
+                _denormalizeDecimals(
+                    _dParams.deposit,
+                    ERC20(_dParams.token).decimals()
+                )) || //Deposit can't be greater than amount.
             (_dParams.amount > 0 &&
                 !IPort(_localPortAddress).isLocalToken(
                     _dParams.hToken,
@@ -59,6 +63,17 @@ library CheckParamsLib {
         }
         return true;
     }
+
+    function _denormalizeDecimals(
+        uint256 _amount,
+        uint8 _decimals
+    ) internal pure returns (uint256) {
+        if (_decimals > 18) {
+            return _amount * (10 ** (_decimals - 18));
+        } else {
+            return _amount / (10 ** (18 - _decimals));
+        }
+    }
 }
 
 /// @title Library for Root Bridge Agent Deployment.
@@ -488,7 +503,10 @@ contract RootBridgeAgent is IRootBridgeAgent {
             _recipient,
             globalAddress,
             _dParams.amount,
-            _dParams.deposit,
+            CheckParamsLib._denormalizeDecimals(
+                _dParams.deposit,
+                ERC20(_dParams.token).decimals()
+            ),
             _fromChain
         );
     }
diff --git a/test/ulysses-omnichain/ArbitrumBranchTest.t.sol b/test/ulysses-omnichain/ArbitrumBranchTest.t.sol
index ea446da..c36d059 100644
--- a/test/ulysses-omnichain/ArbitrumBranchTest.t.sol
+++ b/test/ulysses-omnichain/ArbitrumBranchTest.t.sol
@@ -506,7 +506,7 @@ contract ArbitrumBranchTest is DSTestPlus {
         ftmNativeToken = new MockERC20("underlying token", "UNDER", 18);
 
         // arbitrumNativeAssethToken
-        arbitrumNativeToken = new MockERC20("underlying token", "UNDER", 18);
+        arbitrumNativeToken = new MockERC20("underlying token", "UNDER", 6);
     }
 
     struct OutputParams {
@@ -759,7 +759,7 @@ contract ArbitrumBranchTest is DSTestPlus {
         );
         require(
             MockERC20(newArbitrumAssetGlobalAddress).balanceOf(address(this)) ==
-                100 ether,
+                100e30, //Check for the normalized value
             "User should have 100 global tokens"
         );
     }
@@ -786,6 +786,7 @@ contract ArbitrumBranchTest is DSTestPlus {
 
         arbitrumMulticallBridgeAgent.withdrawFromPort(
             address(newArbitrumAssetGlobalAddress),
+            address(arbitrumNativeToken), //Additional parameter to allow for normalize-denormalize
             100 ether
         );
 
@@ -816,6 +817,7 @@ contract ArbitrumBranchTest is DSTestPlus {
         hevm.expectRevert(abi.encodeWithSignature("UnknownToken()"));
         arbitrumMulticallBridgeAgent.withdrawFromPort(
             address(arbitrumNativeToken),
+            address(arbitrumNativeToken), //Additional parameter to allow for normalize-denormalize
             100 ether
         );
     }
diff --git a/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol b/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol
index 4311662..4d02fe2 100644
--- a/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol
+++ b/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol
@@ -52,7 +52,7 @@ contract BranchBridgeAgentTest is Test {
     function setUp() public {
         wrappedNativeToken = address(new WETH());
 
-        underlyingToken = new MockERC20("underlying token", "UNDER", 18);
+        underlyingToken = new MockERC20("underlying token", "UNDER", 6);
 
         rewardToken = new MockERC20("hermes token", "HERMES", 18);
 

Assessed type

Decimal

Lines of code

https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Upgradeable.sol#L207 https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Upgradeable.sol#L174-L186

Vulnerability details

Ulysses Omnichain - Critical Business Logic Error regarding anyCall Pay Fees on Destination Chain Implementation

Impact

Summary

There is a critical error in the Ulysses Omnichain implementation of the anyCall Pay Fees on Destination / Source Chain 'module'. The error relates to the configuration of the flags parameter and lack of any fees sent to satisfy anyCall protocol fees.

For more details on requirements to calling anyCall properly, see:

• https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Upgradeable.sol#L207
• https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Upgradeable.sol#L174-L186

Details

The implementation of anyCall within Ulysses Omnichain is currently misconfigured to pay fees on the source chain instead of the destination chain. Additionally, the implementation of _performCall in BranchBridgeAgent.sol lacks payment for the unavoidable base or custom "source fees", as is required by the anyCall protocol. Additionally, the project team have communicated through private messaging with the audit team that they intended the protocol to be configured to pay on destination. However, the flags used to configure fee payment for anyCall is set to pay all fees on the source chain.

The impact in relation to the business logic and functionality of the protocol cannot be understated, as in its current configuration any swaps between chains using the anyCall system will fail due to unavailability of gas to pay the implemented/configured fees from the source chain.

While current tests using the anyCall are currently passing using the test suite provided by the project team, this is due to the tests using mocked calls with pre-computed values rather than the actual implementations of anyCall.

It will be demonstrated below with a test that uses deployed anyCall protocol contracts, how the current implementation will fail due to incorrect flag paramters being set.

Proof of Concept

Please see the anyCall v7 integration and best practice documentation here: https://anycall.gitbook.io/anycall/the-latest-version/v7/pay-fees-on-destination-chain

For background,the the function definition for anyCall is as follows:

function anyCall( address _to, bytes calldata _data, uint256 _toChainID, uint256 _flags, bytes calldata )

_flags configures the fallback and fees settings. The are:

0: Gas fee paid on source chain. Fallback not allowed. 2: Gas fee paid on destination chain. Fallback not allowed. 4: Gas fee paid on source chain. Allow fallback 6: Gas fee paid on destination chain. Allow fallback

It can be observed below in the _performCall function within BranchBridgeAgent.sol that executes using the anycallFlags.sol library the contract is configured with `FLAG_ALLOW_FALLBACK = 0x1 << 2' which corresponds with flag 4 in the anyCall documentation to pay fees on the source chain rather than the destination chain.

function _performCall(bytes memory _calldata) internal virtual {
        //Sends message to AnycallProxy
        IAnycallProxy(localAnyCallAddress).anyCall(
            rootBridgeAgentAddress, _calldata, rootChainId, AnycallFlags.FLAG_ALLOW_FALLBACK, ""
        );
    }

Furthermore, the code implentation required to pay fees from the source chain has not been included within the _performCall function. This is likely due to the project team intended the fee structure to be pay on destination chain.

We can see from actual anyCall protocol source (from Anycallv7Upgradeable.sol), anyCall has the invariant of a base source fee, which is not satisfied by the Ulysses Omnichain protocol:

function anyCall( address _to, bytes calldata _data, uint256 _toChainID, uint256 _flags, bytes calldata ) {
    //...

    (string memory _appID, uint256 _srcFees) = IAnycallConfig(config)
        .checkCall(msg.sender, _data, _toChainID, _flags);

    // _srcFees is calculated as roughly: baseFees + _dataLength * feesPerByte
    // for example, a call to BNB chain has a base fee of 1e16 wei (0.01 ether)
    // and a fee-per-byte of 1e10 wei
    _paySrcFees(_srcFees);
    
    //...
}

function _paySrcFees(uint256 fees) internal {
    // fees always > 0, but msg.value is always 0 coming from BranchBridgeAgent::_performCall
    require(msg.value >= fees, "no enough src fee");
    
    // ...
}

The code below tests the exact manner that the ulysses-omnichain protocol calls anyCall, using the forking functionality of foundry to test against actual instances of the anyCall protocol contracts. This proves that in its current implementation, a live, deployed instance of the ulysses-omnichain protocol will fail to send transactions across the anyCall SMPC network.

// @note This is intended to be run in the test/ulysses-omnichain/ folder
// @note for full output run this with `GOERLI_RPC_URL=<alchemy/infura rpc url> forge test --match-contract TestActualOmnichain -vvvvv`
import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";
import "forge-std/Vm.sol";

import {IAnycallProxy} from "../../src/ulysses-omnichain/interfaces/IAnycallProxy.sol";
import {IAnycallConfig} from "../../src/ulysses-omnichain/interfaces/IAnycallConfig.sol";

contract TestActualOmnichain is Test {
    /////////////////////////////////////////////////////////////////////////////////////////////////////
    // @note GOERLI_RPC_URL *must* be set on the command line for this to fork goerli and run correctly!!
    /////////////////////////////////////////////////////////////////////////////////////////////////////
    string GOERLI_RPC_URL = vm.envString("GOERLI_RPC_URL");
    
    uint256 fork;
    address anyCallProxyAddress = 0x965f84D915a9eFa2dD81b653e3AE736555d945f4; // via https://anycall.gitbook.io/anycall/the-latest-version/v7/how-to-integrate-anycall-v7
    address anyCallAddress = 0x7D05D38e6109A3AeEEBf0a570eb8F6856cb4b55E; // via etherscan
    address anyCallConfigAddress = 0x7EA2be2df7BA6E54B1A9C70676f668455E329d29; // via AnyCall.config(), via etherscan
    address anyCallAdmin = 0xfA9dA51631268A30Ec3DDd1CcBf46c65FAD99251; // from AnyCallConfig.admins[0], via etherscan
    
    IAnycallProxy ac = IAnycallProxy(anyCallProxyAddress);
    IAnycallConfig acc = IAnycallConfig(anyCallConfigAddress);
    
    function setUp() public {
        fork = vm.createSelectFork(GOERLI_RPC_URL);
        vm.label(anyCallProxyAddress, "anyCallProxy");
        vm.label(anyCallAddress, "anyCall");
        vm.label(anyCallConfigAddress, "anyCallConfig");
    }
    
    function test_runAnycallOnFork() public {
        address testAddress = address(this);
        address[] memory whitelist = new address[](1);
        whitelist[0] = testAddress;
        
        console.log("Test address", testAddress);

        // whitelist this test so we can make calls to anyCall
        vm.startPrank(anyCallAdmin);
        acc.initAppConfig("testOmnichain", testAddress, testAddress, 0x0, whitelist);
        vm.stopPrank();

        // call AnyCall.anyCall
        // expect request to fail w/ "no enough src fee"

        vm.expectRevert("no enough src fee");
        
        /////////////////////////////////////////////////////////////////////////////
        /////////////////////////////////////////////////////////////////////////////
        // Below is how BranchBridgeAgent.sol calls anyCall, and serves as an example 
        // to prove that the scheme used in the codebase will not work as intended
        /////////////////////////////////////////////////////////////////////////////
        /////////////////////////////////////////////////////////////////////////////

        IAnycallProxy(anyCallProxyAddress).anyCall(
            address(0), // not important, no 0 checks, won't event get a chance to send to addr
            new bytes(0), // not important, won't event get a chance to send
            97, // BNB test chain (has anyCall contracts), won't get a chance to send
            0x4, // pay on src
            "" // unused internally
        );
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Any transaction with anyCall needs to satisfy the base source chain execution fee and a flag update to implement the Pay Fees Destination Chain model that we believe the project team intended.

A code update to _performCall could be:

/** 
 * Sends message to AnycallProxy
 * @dev includes base execution fee and is configured to pay final execution fees on destination
 */
function _performCall(bytes memory _calldata, uint baseExecutionFee) internal virtual {
    IAnycallProxy(localAnyCallAddress).anyCall{value: baseSrcFees}(
        rootBridgeAgentAddress, _calldata, rootChainId, AnycallFlags.FLAG_ALLOW_FALLBACK_DST, ""
    );
}

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L135-L149 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L182-L235 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L353-L361 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyVanilla.sol#L139-L154 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L73-L88

Vulnerability details

High - Talos - V3-Periphery - Missing Slippage Protection in Liquidity Increase Operation in NonfungiblePositionManager.sol

Impact

###Summary

• The lack of slippage protection when calling the increaseLiquidity function, from the NonfungiblePositionManager.sol contract, presents a notable risk to users.

• This could potentially expose users significant price discrepancies, resulting in unfavorable transaction outcomes. With no slippage protection, users will not be protected from changes in price cause by external broad market movements unrelated to the individual trade. Significant and unexpected financial losses incurred to users due to inadequate protections contained witin the MaiaDAO codebase would significantly affect the reputation and continued usability of the protocol.

• Furthermore, the absence of transactional boundaries could potentially open up the system to sandwich attacks, where attackers may manipulate prices to their advantage. Such scenarios may lead to substantial loss of user funds.

• Given the potential financial loss to users and the impact on trust, the severity of this issue is high. Failure to address this vulnerability could potentially result in substantial financial loss and erosion of trust, jeopardizing the platform’s reputation.

Proof of Concept

The increaseLiquidity function is being called with amount0Min and amount1Min set to 0. According to Uniswap’s documentation (https://docs.uniswap.org/contracts/v3/guides/providing-liquidity/increase-liquidity), these parameters should be adjusted in production environments to provide slippage protections. This vulnerability has also been found in other audits, underscoring its relevance and potential for harm.

Locations in the codebase where this is the case include:

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L135-L149

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L182-L235

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L353-L361

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyVanilla.sol#L139-L154

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L73-L88

Attack Vector

Upon observing a trade of asset X for asset Y, an attacker would frontrun the victim trade by also buying asset Y, lets the victim execute the trade, and then backruns (executes after) the victim by trading back the amount gained in the first trade. Intuitively, one uses the knowledge that someone’s going to buy an asset, and that this trade will increase its price, to make a profit. The attacker’s plan is to buy this asset cheap, let the victim buy at an increased price, and then sell the received amount again at a higher price afterwards.

Tools Used

Manual Code Review

Recommended Mitigation Steps

To mitigate this risk, it is advised to implement user protection, such as introducing a minimum return amount for all swap and liquidity provisions/removals across all Router functions. By ensuring that users receive a minimum guaranteed return amount, they can be protected against the potential for significant financial losses due to slippage. Additionally, implementing a slippage check would improve overall platform security, preventing potential front-running and sandwich attacks.

Assessed type

Other

QA Report

Issues Template

Low Risk Template

Gas Template

[L-01] Potential Misuse of transferOwnership

Impact

The renounceOwnership function is overridden to prevent accidental renunciation of ownership, but the transferOwnership function could still be potentially misused to transfer ownership to the zero address or any other unintended address. This could lead to the smart contract becoming ownerless or falling into the wrong hands, leading to a loss of control over the contract’s functionalities, including its funds if any.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L112-L115

In the code snippet provided, we can see that the renounceOwnership function is overridden to disable its functionality:

/// @notice Function being overrriden to prevent mistakenly renouncing ownership.
function renounceOwnership() public payable override onlyOwner {
    revert("Cannot renounce ownership");
}

However, the transferOwnership would still allow the contract owner to transfer ownership to an arbitrary address, including the zero address (address(0)).

Recommended Mitigation

To prevent potential misuse, you should add explicit checks in the transferOwnership function to prevent transferring ownership to the zero address or any other invalid address as per your business logic. The revised function could look like this:

function transferOwnership(address newOwner) public override onlyOwner {
    require(newOwner != address(0), "New owner cannot be the zero address");
    // Add other necessary checks as per your use case
    super.transferOwnership(newOwner);
}

[L-02] Potential Price Manipulation via Flash Loans

Impact

The current implementation uses the spot price of assets directly from the Uniswap V3 pool. This is risky because the pool price can be temporarily manipulated with a flash loan attack, which can result in transactions that are unfair or unfavorable for users. A flash loan attack can happen when someone borrows a large amount of assets and then uses these assets to manipulate the price in a liquidity pool, all within a single transaction.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L674-L696

(uint160 sqrtPriceX96,,,,,,) = IUniswapV3Pool(poolAddress).slot0();

... some code here

try IUniswapV3Pool(poolAddress).swap(
    address(this),
    zeroForOneOnInflow,
    int256(_amount),
    sqrtPriceLimitX96,
    abi.encode(SwapCallbackData({tokenIn: gasTokenGlobalAddress}))
) returns (int256 amount0, int256 amount1)

In this code, the spot price sqrtPriceX96 is taken directly from the Uniswap V3 pool. This price is then used to determine the amounts for the swap.

Recommended Mitigation

To mitigate this risk, it is recommended to use a Time Weighted Average Price (TWAP) instead of the spot price for swaps. TWAP is more resistant to price manipulation as it averages the price over a certain period, making it harder for an attacker to significantly influence the price within a single transaction. Uniswap V3 provides a function to calculate the TWAP, which you can use in your smart contract.

[L-03] Potential Denial of Service Due to large number of gauges

Impact

The current implementation of the gauges function in the ERC20Gauges contract, and its usage in the FlywheelGaugeRewards contract, could potentially lead to a Denial of Service (DoS) if there are too many gauges. The gauges function returns all gauge addresses in an array, which is copied to memory. If there are a large number of gauges, it could consume excessive gas and potentially exceed the block gas limit, rendering the function uncallable.

Proof of Concept

FlywheelGaugeRewards ERC20Gauges Enumerable Set

// FlywheelGaugeRewards.sol
address[] memory gauges = gaugeToken.gauges(); // @audit this is dangerous, since it copies all the gauges._values (addresses) into memory

// ERC20Gauges.sol
function _values(Set storage set) private view returns (bytes32[] memory) {
    return set._values;
}

In this code, the gauges function, which is called in FlywheelGaugeRewards, retrieves all the gauges from the ERC20Gauges contract. This can be problematic if there are many gauges, as it could consume too much memory and gas.

Recommended Mitigation

To mitigate this risk, consider using a pattern such as pagination to retrieve a limited number of gauges at a time. This would avoid excessive gas consumption and potential DoS even if there are a large number of gauges. Here is an example of how pagination can be implemented:

function getGauges(uint256 startIndex, uint256 endIndex) public view returns (address[] memory) {
    require(endIndex >= startIndex, "End index must be greater than or equal to start index");
    uint256 length = endIndex.sub(startIndex);
    address[] memory gauges = new address[](length);
    for (uint256 i = startIndex; i < endIndex; i++) {
        gauges[i - startIndex] = _values[i];
    }
    return gauges;
}

[L-04] User will never enter the if statement when they execute the _checkTimeLimit function multiple times

Impact

The protocol allows users to check if a port strategy has reached its daily management limit:

/**
 * @notice Internal function to check if a Port Strategy has reached its daily management limit.
 *   @param _token address being managed.
 *   @param _amount of token being requested.
 */
function _checkTimeLimit(address _token, uint256 _amount) internal {
    if (block.timestamp - lastManaged[msg.sender][_token] >= 1 days) {
        strategyDailyLimitRemaining[msg.sender][_token] = strategyDailyLimitAmount[msg.sender][_token];
    }
    strategyDailyLimitRemaining[msg.sender][_token] -= _amount;
    lastManaged[msg.sender][_token] = block.timestamp;
}

The _checkTimeLimit function checks whether msg.sender's _token has been lastManaged in a time frame of more than or equal to 1 day.

If the user checks the time limit of a token multiple times, in a time frame of less than 24 hours since the last time it was called, strategyDailyLimitRemaining will never update. That is because _amount is deducted from strategyDailyLimitRemaining, meaning it decreases the daily limit upon repeated executions of the function, eventually preventing further calls as further transactions revert, without any explicit warning.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L194-L196

Recommended Mitigation

Use lastManaged[msg.sender][_token] = block.timestamp; within the if statement.

[L-05] Incomplete Information Emitted in Events

Impact

The currently designed smart contract events do not emit the previous values of the state changes, which could lead to a lack of visibility and traceability when changes are made. This can make it difficult for other developers or auditors to follow the contract’s history and could potentially hinder troubleshooting or understanding of the contract’s behavior over time.

Proof of Concept

Below are examples of events that do not emit the previous values of the state changes:

emit UpdateUserBoost(user, userGaugeBoost);
emit DecrementUserGaugeBoost(msg.sender, gauge, gaugeState.userGaugeBoost);
event IncrementGaugeWeight(address indexed user, address indexed gauge, uint256 weight, uint32 cycleEnd);
event DecrementGaugeWeight(address indexed user, address indexed gauge, uint256 weight, uint32 cycleEnd);

In the above examples, only the new values resulting from the state changes are recorded. Previous state data is not included in the emitted event, which limits the amount of historical information available for these changes.

Recommended Mitigation

It is recommended to modify the events in a way that they also record and emit the previous values during a state change. This would help maintain a more complete history of state changes and improve the contract’s traceability. Here’s an example of how you can include the previous value in an event:

event UpdateUserBoost(address indexed user, uint256 oldValue, uint256 newValue);

In this event, oldValue is the previous value before the state change, and newValue is the value after the state change.

[L-06] Asset Loss When Redeeming Small Amounts

Impact

When dealing with redeeming shares using redeem within TalosBaseStrategy, it has been observed to result in unexpected and significant asset loss relative to the amount deposited. The asset loss seems more pronounced with small amounts and may lead to potential economic and accounting exploits or financial losses for users.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L238-L29

Two test cases have been written to illustrate this issue. In the first case, if shares are redeemed in smaller amounts across multiple transactions, the user seems to get a lower return. In the second case, if all shares are redeemed at once, the user receives an expected return.

function testWithdrawSmallAmount() public {
    uint256 amount0Desired = 10;

    (uint256 totalShares,uint256 amount0_, uint256 amount1_) = deposit(amount0Desired, amount0Desired, user1);
    assertEq(amount0_,10);
    assertEq(amount1_,6);

    uint256 sharesRedeemed = totalShares/5;
    assertEq(sharesRedeemed*5, totalShares);

    for(int i = 0; i < 5; i++){
        (uint256 amount0, uint256 amount1) =  withdraw(sharesRedeemed, user1);
        assertEq(amount0,1);
        assertEq(amount1,1);
    }
function testWithdrawAllSmall() public {
    uint256 amount0Desired = 10;

    (uint256 totalShares,uint256 amount0_,uint256 amount1_) = deposit(amount0Desired, amount0Desired, user1);
    assertEq(amount0_,10);
    assertEq(amount1_,6);

    (uint256 amount0, uint256 amount1) =  withdraw(totalShares, user1);
    assertEq(amount0,9);
    assertEq(amount1,5);
}

In testWithdrawSmallAmount(), redeeming 1/5 of the shares 5 times yields 1 of each token every time, totaling 5 of each. In contrast, in testWithdrawAllSmall(), redeeming all shares at once yields 9 of one token and 5 of another, revealing an apparent loss compared to the expected amounts. That is a 55% loss of assets.

Recommended Mitigation

Ensure that users do not withdraw only a small amount of shares.

[L-07] Lack of OnlyOwner Modifier in claimProtocolFees Function

Impact

The claimProtocolFees function, located within the admin logic section of the contract, lacks the onlyOwner modifier. This omission exposes a potential vulnerability and creates a vector for unauthorized access and manipulation of the protocol fees. Although it may not directly result in a loss of funds, the absence of the onlyOwner modifier can lead to unauthorized changes in the contract’s state and disrupt the intended distribution and utilization of the protocol fees.

Proof of Concept

While no specific exploit scenario or attacker contract is presented, the lack of the onlyOwner modifier within the claimProtocolFees function enables any address to call the function and transfer the protocol fees to the owner. As a result, an unauthorized party could manipulate the state of the contract and potentially disrupt the balance and intended distribution of the protocol fees. This introduces a vector of attack and compromises the control and governance mechanisms in place.

Recommended Mitigation

To address this vulnerability and enhance the security of the contract’s admin logic, it is strongly recommended to add the onlyOwner modifier to the claimProtocolFees function. By doing so, only the contract owner will have the authority to execute the function and transfer the protocol fees.