`DoS` Attack via Small Donation in `VaultBitcoinWallet`

[hats-bug-reporter[bot]]

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

Description: Description\ The _onActionDeposit function in the VaultBitcoinWallet contract is vulnerable to a Denial of Service attack. A malicious actor can exploit this vulnerability by sending a very small amount of Bitcoin such that the value is equal to or less than the sum of protocolFees and importFees.

 uint64 protocolFees = value * depositFee / 1000;
        if (isExcludedFromFees[destination]) {
            protocolFees = 0;
        }

        uint64 importFees = ((BYTES_PER_INCOMING_TRANSFER + INPUT_EXTRA_FIXED_BYTES_FEE) * satoshiPerByte);
        protocolFees += importFees;

        uint64 valueAfterFees = value - protocolFees;

        btcToken.mint(destination, valueAfterFees);//@audit-here if the value = protocol fees then the function revert due to `mint(destination,0)`

This can cause the function to revert, now due to this small donation attack,The prover will be unable to process legitimate deposits due to repeated reversion in the _onActionDeposit function.

Attack Scenario\

Attacker Sends Small Donation: The attacker sends a Bitcoin deposit with a value that is less than or equal to the sum of protocolFees and importFees. For instance, if protocolFees is 1000 satoshis and importFees is 500 satoshis, the attacker sends a deposit of 1500 satoshis or less.

Prover Calls processDeposit: The prover calls the processDeposit function in the BitcoinAbstractWallet contract to verify and process the transaction. The processDeposit function internally calls _processDeposit, which then calls _onActionDeposit.

Function Reversion: Inside _onActionDeposit, the valueAfterFees calculation results in zero or a negative value. This causes the subsequent operations, such as minting tokens and executing hooks, to fail, leading to the function reverting.

DoS Attack: The attacker repeatedly sends such small deposits, causing the _onActionDeposit function to revert each time. This prevents the prover from processing legitimate deposits, effectively leading to a DoS attack.

Attachments

1. Proof of Concept (PoC) File

   function _onActionDeposit(
       uint64 value,
       bytes memory _vaultScriptHash,
       bytes memory _recoveryData
   ) internal returns (bytes32) {
       (uint256 _keyIndex, bytes memory _encryptedRecoveryData) = abi.decode(_recoveryData, (uint256, bytes));
       bytes memory recoveryData = _decryptPayload(_keyIndex, _encryptedRecoveryData);
   
       if (_changeSecretDerivationRoot == bytes32(0)) {
           _changeSecretDerivationRoot = _random(keccak256(
               abi.encodePacked(
                   value, _vaultScriptHash, _recoveryData, block.number
               )
           ));
       }
   
       (uint256 offchainPubKeyIndex,, address destination, bytes memory data) = abi.decode(
           recoveryData,
           (uint256, bytes32, address, bytes)
       );
   
       require(bytes20(_vaultScriptHash) == _keyDataToScriptHash(offchainPubKeyIndex, _keyIndex, keccak256(recoveryData)), "IR");
   
       uint64 protocolFees = value * depositFee / 1000;
       if (isExcludedFromFees[destination]) {
           protocolFees = 0;
       }
   
       uint64 importFees = ((BYTES_PER_INCOMING_TRANSFER + INPUT_EXTRA_FIXED_BYTES_FEE) * satoshiPerByte);
       protocolFees += importFees;
   
       uint64 valueAfterFees = value - protocolFees;
   
       btcToken.mint(destination, valueAfterFees);
       if ((protocolFees - importFees) > 0 && destination != REFUEL_VAULT_ADDRESS) {//@audit-DONATION ATTACK
           btcToken.mint(owner(), protocolFees - importFees);
       }
   
       if (hooks[destination] && destination != REFUEL_VAULT_ADDRESS) {
           IVaultBitcoinWalletHook(destination).hook(valueAfterFees, data);
       }
   
       bytes32 _secret = _getKeyPairSeed(_keyIndex, keccak256(recoveryData));
       bytes32 _keyImage = keccak256(abi.encodePacked(_secret));
       _secrets[_keyImage] = _secret;
   
       if (destination == REFUEL_VAULT_ADDRESS) {
           _isRefuelInputSecret[_keyImage] = true;
       }
   
       _inputKeyImageToOffchainSignerPubKeyIndex[_keyImage] = offchainPubKeyIndex;
   
       _updateKey(keccak256(abi.encodePacked(
           value,
           _vaultScriptHash,
           _recoveryData,
           block.number
       )));
   
       return _keyImage;
   }

2. Revised Code File (Optional)

[party-for-illuminati]

It is related to prover's implementation which will have amount filtration

[batmanBinary]

It is related to prover's implementation which will have amount filtration

@party-for-illuminati , could you please provide a brief explanation of what "amount filtration" means in this context? I'm not entirely sure what you are referring to and would appreciate some clarification.

[party-for-illuminati]

It is related to prover's implementation which will have amount filtration

@party-for-illuminati , could you please provide a brief explanation of what "amount filtration" means in this context? I'm not entirely sure what you are referring to and would appreciate some clarification.

Relayer won't prove transactions with amount lower than X where X is a configured minimum which is also considered on the frontend side, there is no need to fix it on the contracts side in my opinion

[batmanBinary]

hey @party-for-illuminati ,Thank you for your prompt response.However, I believe addressing this issue at the contract level is still necessary for the following reasons:

• it's crucial to assume that not all interactions will go through the intended frontend or relayer. Users or malicious actors might interact directly with the contract. By enforcing a minimum donation amount within the contract, we ensure that the contract remains secure regardless of how it is accessed.

• Relying solely on the relayer and frontend configurations for security introduces a single point of failure. If these layers are compromised or misconfigured, the contract remains vulnerable.

• Configurations and frontend implementations can change over time, and there’s always a risk of human error during updates. By embedding the minimum donation logic within the contract, It make the security measure immutable

• While the relayer and frontend configurations are valuable, they should complement, not replace, contract-level security measures.and let's also not forget that the frontend can also or maybe bypassed by attacker

I hope this clarifies the importance of addressing the issue at the contract level. I look forward to your thoughts on this matter.

[batmanBinary]

@party-for-illuminati ??

[batmanBinary]

hey @party-for-illuminati ,waiting for the reply.