`ackTransaction` Function Always Reverts, Resulting in Inability to Process Bitcoin Transaction Proofs

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xf5f51c5cef88a7d2c6461dcbf472cb9718a38acb7bb7f4284450e43c27a48764 Severity: high

Description: Description\ The ackTransaction function in the BitcoinProver contract is responsible for acknowledging and processing a Bitcoin transaction proof. Here, the ackTransaction() calls the processDeposit() function in the BitcoinAbstractWallet.sol through an interface.

function ackTransaction(
        TEERollup.FullComputationsProof memory txProof,
        IBitcoinDepositProcessingCallback callback,
        bytes memory _data
    ) public onlyRelayer {
        require(verifyComputations(txProof), "Invalid proof");

        (uint8 actionCode, IBitcoinDepositProcessingCallback.Transaction memory _tx) = abi.decode(
            txProof.partialProof.computationsResult,
            (uint8, IBitcoinDepositProcessingCallback.Transaction)
        );

        require(actionCode == uint8(ProvingAction.Transaction), "Invalid action code");
        callback.processDeposit(_tx, _data);<----
    }

The processDeposit() function checks for the caller using require(msg.sender == prover);.

 function processDeposit(
        Transaction memory _tx,
        bytes memory _data
    ) public override {
        require(msg.sender == prover);<---
        _processDeposit(_tx, _data);
    }

Since the function is called from the BitcoinProver contract, this check passes, and it then calls the _processDeposit() function internally.

However, the issue arises when the _processDeposit() function calls the _onDeposit function.

function _processDeposit(Transaction memory _tx, bytes memory _data) internal {
        bytes memory btcAddress = new bytes(0);
        bytes4 _scriptId = bytes4(0);
        for (uint i = 0; i < scripts.length; i++) {
            btcAddress = scripts[i].deserialize(_tx.transaction.script);
            _scriptId = scripts[i].id();
            if (btcAddress.length > 0) {
                break;
            }
        }

        require(btcAddress.length > 0, "UIS");
        bytes32 _keyImage = _onDeposit(_scriptId, _tx.transaction.value, btcAddress, _data, _tx);<-------

        bytes32 inputHash = _inputHash(_tx.txHash, _tx.txOutIndex);

        require(!inputs[inputHash].recorded, "IAA");
        inputs[inputHash] = InputMetadata(true, true, _tx.txHash, _tx.txOutIndex, _keyImage, _tx.transaction.value);

        emit Deposit(inputHash, _tx.transaction.value, _keyImage);
    }

This function is responsible for handling deposits based on the provided script ID, value, vault script hash, recovery data, and transaction details. Since the BitcoinAbstractWallet.sol contract is abstract, the _onDeposit() function is not logically implemented (it is just declared).

 function _onDeposit(
        bytes4 scriptId,
        uint64 value,
        bytes memory to,
        bytes memory recoveryData,
        Transaction memory _tx
    ) internal virtual returns (bytes32);

This function is supposed to be overridden in the BitcoinProver.sol (the calling contract), but there is no logical implementation of the _onDeposit() function in the BitcoinProver.sol. As a result, when the ackTransaction() function is called, it always reverts. In fact, the _onDeposit function is defined in the VaultBitcoinWallet.sol, which makes no sense and acts like a dead function.

Revised Code File (Optional)

• By implementing and overriding the _onDeposit function in the BitcoinProver contract, you ensure that the ackTransaction function can successfully process deposits without reverting.

  function _onDeposit(
      bytes4 scriptId,
      uint64 value,
      bytes memory _vaultScriptHash,
      bytes memory _recoveryData,
      Transaction memory _tx
  ) internal virtual override returns (bytes32) {
      require(scriptId == bytes4(keccak256(abi.encodePacked(type(ScriptP2SH).name))), "IS");
  
      (VaultTransactionType _type, bytes memory _data) = abi.decode(_recoveryData, (VaultTransactionType, bytes));
  
      if (_type == VaultTransactionType.Deposit) {
          return _onActionDeposit(value, _vaultScriptHash, _data);
      } else if (_type == VaultTransactionType.Outbound) {
          return _onActionOutbound(value, _vaultScriptHash, _tx);
      } else {
          revert("IAT");
      }
  }

[batmanBinary]

@rotcivegaf ?

[rotcivegaf]

The solidity compiler itself will throw an error when trying to compile a contract that has an unimplemented function

[batmanBinary]

hey @rotcivegaf ,that itself is a issue right? and infact the _onDeposit function is defined in the VaultBitcoinWallet.sol, which makes no sense and acts like a dead function

[party-for-illuminati]

hey @rotcivegaf ,that itself is a issue right? and infact the _onDeposit function is defined in the VaultBitcoinWallet.sol, which makes no sense and acts like a dead function

processDeposit same as _onDeposit are implemented on the VaultBitcoinWallet side, there is no issue. Can you just try to compile it please?

[batmanBinary]

hey @party-for-illuminati ,

processDeposit same as _onDeposit are implemented on the VaultBitcoinWallet side

processDeposit() is not implemented in VaultBitcoinWallet side,it is correctly implemented in the BitcoinAbstractWallet as expected but the issue here is the _onDeposit() function which is used in the processDeposit() internally should be either implemented in the BitcoinAbstractWallet or BitcoinProver,but the function is not implemented in neither contracts indeed this function is implemented in the VaultBitcoinWallet which is not called by any function hence acts like dead function.

Does this seem clear to you? Can you please verify it?

[party-for-illuminati]

hey @party-for-illuminati ,

processDeposit same as _onDeposit are implemented on the VaultBitcoinWallet side

processDeposit() is not implemented in VaultBitcoinWallet side,it is correctly implemented in the BitcoinAbstractWallet as expected but the issue here is the _onDeposit() function which is used in the processDeposit() internally should be either implemented in the BitcoinAbstractWallet or BitcoinProver,but the function is not implemented in neither contracts indeed this function is implemented in the VaultBitcoinWallet which is not called by any function hence acts like dead function.

Does this seem clear to you? Can you please verify it?

This is not correct.

1. onDeposit shouldn't and cannot be implemented on BitcoinProver side

2. VaultBitcoinWallet is inherited from BitcoinAbstractWallet

[party-for-illuminati]

As well as _onDeposit is implemented in VaultBitcoinWallet