Bfx.withdraw() failed transactions are open to replay attack

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 97Sabit Submission hash (on-chain): 0x6434e495e524778491f63dce485d15adf523ba5850048cb4a42e0f656be7878f Severity: high

Description: Description\ In the current implementation of withdraw(), the function uses a low-level call to make transfers through tokenCall(). Any failed transactions would revert and processedWithdrawals[id] == false would remain as it is.

As a result, the same transaction can be replayed by anyone using the same signature. It is worthy to note that implementation of EIP712 itself doesn’t protect against replay attacks - “it does not include replay protection”.

See: https://eips.ethereum.org/EIPS/eip-712

Here’s also a similar finding: https://solodit.xyz/issues/h-04-eip712metatransactionexecutemetatransaction-failed-txs-are-open-to-replay-attacks-code4rena-rolla-rolla-contest-git

1. Proof of Concept (PoC) File

https://github.com/hats-finance/Blast-Futures-Exchange-0x97895c329b950755566ddcdad3395caaea395074/blob/d9b402f5124f1470f979ed9a6d010d89988f6dee/foundry/src/Bfx.sol#L48-L68

https://github.com/hats-finance/Blast-Futures-Exchange-0x97895c329b950755566ddcdad3395caaea395074/blob/d9b402f5124f1470f979ed9a6d010d89988f6dee/foundry/src/Bfx.sol#L100

https://github.com/hats-finance/Blast-Futures-Exchange-0x97895c329b950755566ddcdad3395caaea395074/blob/d9b402f5124f1470f979ed9a6d010d89988f6dee/foundry/src/Bfx.sol#L107-L113

[alex-sumner]

In the case that the withdraw transaction fails and is reverted, no withdrawal has taken place and the user should be able to try again. This is a retry, not a replay. A replay vulnerability would only exist if it was possible to successfully submit the same withdrawal more than once, thus obtaining funds more than once. This is not possible.

[ololade97]

The issue is when the withdraw is submitted and it fails, the signature is visible on the blockchain. It can be replayed by an attacker before the real owner resubmits it.

[ololade97]

Note that there is no protection such as nonce against submitting the same signature again when still valid.

[alex-sumner]

But in that case the withdrawal will go to the real owner, not the attacker. The attacker will just pay for the gas.

[ololade97]

But in that case the withdrawal will go to the real owner, not the attacker. The attacker will just pay for the gas.

The amount is sent to trader. trader is a parameter passed to the function specified by the caller. So, the attacker is at liberty to send the amount to any address.

makeTransfer(trader, amount);

[alex-sumner]

Changing the trader will invalidate the signature.

[ololade97]

The signature is different from the trader parameter input.

Assuming the real owner decides to send the amount to a different trader from the one he first specified when the withdrawal failed, this won't invalidate the signature.

[alex-sumner]

The signature checking involves hashing the input data, including the trader, and some other values, then determining whether that hash was signed by the correct singer address to produce the provided signature. If you change the trader address this check will fail and your transaction will be reverted with an "INVALID_SIGNER" message. You can only re-use a signature if it has not already been processed (that is, if this is the first attempt to use it, or if all previous attempts failed and were reverted) and you have not changed anything about the withdrawal (amount, recipient, etc.).

[ololade97]

Ok, thanks.