Predictable and Colliding Transfer IDs Due to Zero idSeed

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xbcfb0a79248caedd5207e493043124072f9f26cfc60a88888f24f1e6c5517385 Severity: medium

Description: Description\ The withdraw function in the VaultBitcoinWallet contract allows the idSeed parameter to be 0, which can lead to predictable and colliding transfer IDs. This issue arises because the idSeed is used in the keccak256 hash calculation to generate a unique _transferId for each transaction. If idSeed is 0, the generated _transferId will have reduced entropy, making it easier to predict or collide with other transfer IDs, especially if other parameters (to and amount) are the same across different transactions

Attack Scenario\ Consider the following example where two users initiate a withdrawal with the same to address and amount, but with idSeed set to 0:

1. User A: to = "1BitcoinAddress" amount = 1000 satoshis idSeed = 0
2. User B: to = "1BitcoinAddress" amount = 1000 satoshis idSeed = 0

Both transactions will generate the same _transferId: bytes32 _transferId = keccak256(abi.encodePacked(0, "1BitcoinAddress", 1000)); This collision can cause one transaction to overwrite the other in the OutgoingQueue, leading to loss of transaction data and potential disputes.

Attachments

1. Proof of Concept (PoC) File

   function withdraw(bytes memory to, uint64 amount, uint64 minReceiveAmount, bytes32 idSeed) public {
       uint64 amountAfterNetworkFee = amount - (BYTES_PER_OUTGOING_TRANSFER * satoshiPerByte);
       require(amountAfterNetworkFee >= minWithdrawalLimit, "AFL");
   
       uint64 protocolFees = amountAfterNetworkFee * withdrawalFee / 1000;
       if (isExcludedFromFees[msg.sender]) {
           protocolFees = 0;
       }
   
       uint64 amountAfterFee = amountAfterNetworkFee - protocolFees;
       require(amountAfterFee >= minReceiveAmount, "FTH");
   
       btcToken.burn(msg.sender, amount);
       if (protocolFees > 0) {
           btcToken.mint(owner(), protocolFees);
       }
   
       bytes32 _transferId = keccak256(abi.encodePacked(idSeed, to, amount));
       queue.push(
           OutgoingQueue.OutgoingTransfer(
               BitcoinUtils.resolveLockingScript(to, _isTestnet(), workingScriptSet),
               amountAfterFee,
               _transferId
           )
       );
   }

   Impact

2. Predictable Transfer IDs: Using 0 as idSeed reduces the uniqueness of the _transferId, making it more predictable. Malicious actors could exploit this predictability to track or interfere with transactions.

3. Collision of Transfer IDs: Multiple transactions with the same to address and amount but with idSeed set to 0 will generate the same _transferId. This can lead to overwriting or loss of transaction data in the OutgoingQueue, causing potential disputes and inconsistencies.

4. Security Risks: Predictable or colliding transfer IDs pose a security risk. Malicious actors might exploit these predictable IDs to manipulate or disrupt the transaction process, compromising the integrity and security of the system.

5. Revised Code File (Optional)

   • To mitigate these issues, add a check to ensure that idSeed is always non-zero. This will maintain the entropy and uniqueness of the generated _transferId, reducing the risk of predictable or colliding transfer IDs.

[party-for-illuminati]

It can't lead to any issues and being used for tracking on the frontend side only