Use abi.encodePacked in _sign() function similar to as done by Sapphire

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0xeb5f742f50e72bd0b9a2ab9f05147ae2a027e65f13e3cceb384b38a88d86c2ef
Severity: low

Description:
Description\

AbstractTxSerializer.sol, _sign() has used bytes.concat to concatenate the input bytes32 _sigHash:

    function _sign(
        bytes32 _inputId,
        bytes32 _sigHash
    ) internal view returns (bytes memory sig) {
        (, bytes memory privKey) = _getKeyPair(_inputId);

        sig = Sapphire.sign(
            Sapphire.SigningAlg.Secp256k1PrehashedSha256,
            privKey,
@>            bytes.concat(_sigHash),
            ""
        );
    }

Affected code:

illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf/packages/contracts/contracts/illuminex/xengine/chains/btc/wallet/tx-serializer/AbstractTxSerializer.sol

Line 90 in 3ad7c2a

bytes.concat(_sigHash),

The issue is that, for such _sign implementation, use of bytes.concat() is not recommended as bytes.concat() per solidity documentation is used in case:

the bytes.concat function can concatenate an arbitrary number of bytes or bytes1 ... bytes32 values. The function returns a single bytes memory array that contains the contents of the arguments without padding.

_sigHash is a single input in bytes32 form and its not arbitrary number of bytes to use the bytes.concate().

Further, Sapphire has used abi.encodePacked() instead of bytes.concat() wherever Sapphire.sign() function used.

For example in Sapphire's EthereumUtils.sol , see below how bytes32 digest has used abi.encodePacked to concatenate the bytes.

    function sign(
        address pubkeyAddr,
        bytes32 secretKey,
        bytes32 digest
    ) internal view returns (SignatureRSV memory rsv) {
        bytes memory signature = Sapphire.sign(
            Sapphire.SigningAlg.Secp256k1PrehashedKeccak256,
            abi.encodePacked(secretKey),
@>          abi.encodePacked(digest),
            ""
        );

        rsv = splitDERSignature(signature);

        recoverV(pubkeyAddr, digest, rsv);
    }

Therefore, its recommended to use abi.encodePacked().

Recommendation to fix
Consider below changes in AbstractTxSerializer.sol:

    function _sign(
        bytes32 _inputId,
        bytes32 _sigHash
    ) internal view returns (bytes memory sig) {
        (, bytes memory privKey) = _getKeyPair(_inputId);

        sig = Sapphire.sign(
            Sapphire.SigningAlg.Secp256k1PrehashedSha256,
            privKey,
-           bytes.concat(_sigHash),
+          abi.encodePacked(_sigHash),
            ""
        );
    }

[party-for-illuminati]

Informational