c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L350-L353

Vulnerability details

Impact

Attackers can gain control of User's Virtual Accounts and steal all the assets these accounts hold in the Root environment

Proof of Concept

When sending signed messages from a Branch to Root, the RootBridgeAgent contract calls the RootPort::fetchVirtualAccount() to get the virtual account that is assigned in the Root environment to the address who initiated the call in the SrcBranch, and if that address doesn't have assigned a virtual account yet, it proceeds to create one and assign it.
The problem is that the fetchVirtualAccount() function solely relies on the address of the caller in the SrcBranch, but it doesn't take into account from which Branch the call comes.

BranchBridgeAgent.sol

function callOutSignedAndBridge(
...
) external payable override lock {
...
//Encode Data for cross-chain call.
bytes memory payload = abi.encodePacked(
_hasFallbackToggled ? bytes1(0x85) : bytes1(0x05),
//@audit-info => Encodes the address of the caller in the Branch and sends it to the RootBridgeAgent
//@audit-info => This address will be used to fetch the VirtualAccount assigned to it!
msg.sender,
_depositNonce,
_dParams.hToken,
_dParams.token,
_dParams.amount,
_dParams.deposit,
_params
);
}

RootBridgeAgent.sol


function lzReceiveNonBlocking(
...

) public override requiresEndpoint(_endpoint, _srcChainId, _srcAddress) { ... ... ... else if (_payload[0] == 0x04) { // Parse deposit nonce nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED]));

  //Check if tx has already been executed
  if (executionState[_srcChainId][nonce] != STATUS_READY) {
      revert AlreadyExecutedTransaction();
  }

  //@audit-info => Reads the address of the msg.sender in the BranchBridgeAgent and forwards that address to the RootPort::fetchVirtualAccount()
  // Get User Virtual Account
  VirtualAccount userAccount = IPort(localPortAddress).fetchVirtualAccount(
      address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED])))
  );

  // Toggle Router Virtual Account use for tx execution
  IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

...
...

} ... ... }

> RootPort.sol
```solidity
//@audit-info => Receives from the RootBridgeAgent contract the address of the caller in the BranchBridgeAgent contract
//@audit-info => Fetches the VirtualAccount assigned to the _user address regardless from what Branch the call came from
function fetchVirtualAccount(address _user) external override returns (VirtualAccount account) {
    account = getUserAccount[_user];
    if (address(account) == address(0)) account = addVirtualAccount(_user);
}

Like Example, Let's suppose that a user uses a MultiSigWallet contract to deposit tokens from Avax to Root, in the RootBridgeAgent contract, the address of the MultisigWallet will be used to create a Virtual Account, and all the globalTokens that were bridged will be deposited in this Virtual Account.
- Now, the problem is that the address of the MultisigWallet, might not be controlled by the same user on a different chain, For example, in Polygon, an attacker could gain control of the address of the same address of the MultisigWallet that was used to deposit tokens from Avax in the Root environment, an attacker can send a signed message from Polygon using the same address of the MultisigWallet that deposited tokens from Avax, to the Root environment, requesting to withdraw the assets that the Virtual Account is holding in the Root environment to the Polygon Branch.
  - When the message is processed by the Root environment, the address that will be used to obtain the Virtual Account will be the address that initiated the call in Polygon, which will be the same address of the user's MultisigWallet contract who deposited the assets from Avax, but the Root environment, when fetching the virtual account, makes no distinctions between the branches, thus, it will give access to the Virtual Account of the attacker's caller address and process the message in the Root environment.
  - As a result, an attacker can gain control of the Virtual Account of an account contract that was used to deposit assets from a chain into Root, by gaining control of the same address of the account contract that deposited the assets in a different chain.

As explained in detail on this article written by Rekt, it is possible to gain control of the same address for contract accounts in a different chain, especially for those contract accounts that are deployed using the Gnosis Safe contracts:

Tools Used

Manual Audit & Article wrote by Rekt

Recommended Mitigation Steps

The recommendation is to add some logic that validates if the caller address in the BranchBridgeAgent is a contract account or an EOA, and if it's a contract account, send a special flag as part of the crosschain message, so that the RootBridgeAgent contract can know if the caller in the SrcBranch it's a contract or an EOA.
- If the caller is an EOA, the caller's address can be assigned as the Virtual Account owner on all the chains, for EOAs there are no problems.
- But, if the caller is a Contract Account, when fetching the virtual account, forward the SrcChain, and if a Virtual Account is created, just authorize the caller address on the SrcBranch as the owner for that Virtual Account, in this way, only the contract account in the SrcBranch can access the Virtual Account in the Root environment.
- Make sure to use the srcChainId to validate if the caller is an owner of the Virtual Account!

Assessed type

Access Control

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

c4-sponsor commented 1 year ago

0xBugsy (sponsor) disputed

c4-sponsor commented 1 year ago

0xLightt (sponsor) confirmed

0xLightt commented 1 year ago

Contracts should not use Virtual Accounts and deploying a specific router for contract usage is most likely the safest option.