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

code-423n4 / 2023-05-maia-findings

Ulysses Omnichain support for tokens with other than 18 decimals is fundamentally flawed #498