Insufficient Check for Duplicate Witness Signatures

[hats-bug-reporter[bot]]

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

Description: Description

The current implementation of the verifyComputations function in the TEERollup contract checks for duplicate witness signatures by comparing the keccak256 hash of public keys. This approach is computationally expensive and can be optimized. An attacker could exploit this inefficiency to increase gas costs unnecessarily, potentially leading to denial-of-service (DoS) attacks due to high gas consumption.

Attachments

1. Proof of Concept (PoC) File

   function verifyComputations(FullComputationsProof memory fullProof) public view returns (bool) {
       bool isContractSignatureValid = Sapphire.verify(
           Sapphire.SigningAlg.Secp256k1PrehashedKeccak256,
           _keyPair.publicKey,
           abi.encodePacked(keccak256(fullProof.partialProof.computationsResult)),
           "",
           fullProof.partialProof.contractSignature
       );
   
       if (!isContractSignatureValid) {
           return false;
       }
   
       if (fullProof.witnessSignatures.length < minWitnessSignatures) {
           return false;
       }
   
       bytes[] memory _usedPubKeys = new bytes[](fullProof.witnessSignatures.length);
   
       for (uint i = 0; i < fullProof.witnessSignatures.length; i++) {
           if (!witnessPublicKeysSet[fullProof.witnessSignatures[i].publicKey]) {
               return false;
           }
   
           for (uint j = 0; j < _usedPubKeys.length; j++) {
               if (keccak256(_usedPubKeys[j]) == keccak256(fullProof.witnessSignatures[i].publicKey)) {
                   return false;
               }
           }
   
           _usedPubKeys[i] = fullProof.witnessSignatures[i].publicKey;
   
           bool isWitnessSignatureValid = Sapphire.verify(
               Sapphire.SigningAlg.Secp256k1PrehashedKeccak256,
               fullProof.witnessSignatures[i].publicKey,
               abi.encodePacked(keccak256(fullProof.partialProof.computationsResult)),
               "",
               fullProof.witnessSignatures[i].signature
           );
   
           if (!isWitnessSignatureValid) {
               return false;
           }
       }
   
       return true;
   }

Recommendation to fix

Use a more efficient data structure, such as a mapping or a set, to track used public keys. This will reduce the computational overhead and gas costs associated with checking for duplicate witness signatures, enhancing the contract's performance and mitigating potential DoS attacks.

[rotcivegaf]

It's a gas opt or medium issue?

[Top88Star]

It's a gas opt or medium issue?

@rotcivegaf

Medium since the function is vulnerable to gas griefing attacks primarily due to the nested loops and the verification logic that can be manipulated to consume excessive gas. Signature verification is computationally expensive and thus the function is subject to the upper-bound gas limit on the block.

This can cause the transaction to exceed the block gas limit, making the function execution fail consistently,

verifyComputations function is called on several places: getBlockChunkProof, getBlockChunkRollupProof, ackAnchorBlock and ackTransaction which is part of the user's deposit process:

  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);
    }

[party-for-illuminati]

It's a gas opt or medium issue?

@rotcivegaf

Medium since the function is vulnerable to gas griefing attacks primarily due to the nested loops and the verification logic that can be manipulated to consume excessive gas. Signature verification is computationally expensive and thus the function is subject to the upper-bound gas limit on the block.

This can cause the transaction to exceed the block gas limit, making the function execution fail consistently,

verifyComputations function is called on several places: getBlockChunkProof, getBlockChunkRollupProof, ackAnchorBlock and ackTransaction which is part of the user's deposit process:

  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);
    }

It isn't an issue, nor an attack scenario. There won't be this many witnesses for it to become an issue, as well as witnesses amount is controlled by the owner