`verifyComputations()` will always fail and return false due to incorrect implementation

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

Description: Description\

TEERollup is a base contract which is inherited by BitcoinProver contract.

This issue is about incorrect implementation of verifyComputations() function which would always return false for given input arguments. verifyComputations() function is implemented as:

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

To check, contract signature validation, the above function has implemented below logic:

        bool isContractSignatureValid = Sapphire.verify(
            Sapphire.SigningAlg.Secp256k1PrehashedKeccak256,
            _keyPair.publicKey,
            abi.encodePacked(keccak256(fullProof.partialProof.computationsResult)),
            "",
            fullProof.partialProof.contractSignature
        );

        if (!isContractSignatureValid) {
            return false;
        }

and To check, Witness signature validation, the above function has implemented below logic:

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

If both of these conditions pass then it will return true which computations are verfied otherwise it will return false.

The issue is about both of these condition which will ALWAYS return false due to their incorrect implementation. Both of these conditions have used Sapphire.verify() function from oasisprotocol's Sapphire.sol contract.

verify() is implemented as:

    function verify(
        SigningAlg alg,
        bytes memory publicKey,
        bytes memory contextOrHash,
        bytes memory message,
        bytes memory signature
    ) internal view returns (bool verified) {
        (bool success, bytes memory v) = VERIFY_DIGEST.staticcall(
            abi.encode(alg, publicKey, contextOrHash, message, signature)
        );
        require(success, "verify: failed");
        return abi.decode(v, (bool));
    }

Reference: https://github.com/oasisprotocol/sapphire-paratime/blob/2ada53a4a25034f123a37dadd25981c9267874d3/contracts/contracts/Sapphire.sol#L478

It can be seen that, input arguments are taken in bytes which is partially followed in above both conditions while validating contract and witness signatures.

contextOrHash is passed as bytes argument in verify() but the verifyComputations() has passed this param as bytes32. This is because of below line of code:

This encode the computationsResult inbytes32 which is encoded with keccak256. This would be incorrect input argument which will restrict the byte size of computationsResult to bytes32 which is not expected in verify() function.

It should be noted that, computationsResult can be directly used which is in bytes data format and does not need to be encoded with keccak256.

    struct PartialComputationsProof {
        bytes computationsResult;    @audit // already in bytes data type
        bytes contractSignature;
    }

Due to this issue, verifyComputations() will return false as verify() function will always fail.

The following functions have used verifyComputations() in BitcoinProver.sl

1) getBlockChunkProof() 2) getBlockChunkRollupProof() 3) ackTransaction() 4) ackAnchorBlock()

Inshort, whole contracts functionalities are broken due to incorrect implementation of verifyComputations(). This would make the contracts functionalities useless and all of these functions will always revert with their string message.

Recommendation to fix\ Consider below changes :

    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.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.partialProof.computationsResult,
                "",
                fullProof.witnessSignatures[i].signature
            );

            if (!isWitnessSignatureValid) {
                return false;
            }
        }

        return true;
    }

hats-finance / illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf

`verifyComputations()` will always fail and return false due to incorrect implementation #30