The ZrSign contract contains the setupBaseFee and setupNetworkFee. These functions allow the owner to set the base and network fee for the contract. These fees are necessary for calculating the value that the caller has to provide to the contract to pay for the transaction, particularly verified inside the keyFee() and sigFee() modifiers.

However, none of these functions verify that the proposed fees fall within a threshold to avoid potential issues, i.e. integer overflow, when performing operations on these values inside the sigFee() modifier:

uint256 networkFee = payload.length * $._networkFee;
uint256 totalFee = $._baseFee + networkFee;

Consider adding checks to the setupBaseFee() and setupNetworkFee() functions to verify that the proposed fees fall within a threshold.

Remediation Plan

RISK ACCEPTED: The ZenRock Labs team made a business decision to accept the risk of this finding and not alter the contracts.

The _sigReq function, utilized by external functions zrSignHash, zrSignData, and zrSignTx, handles hashing and signing data operations. This function is accessible to any user via the mentioned external functions. However, it does not validate the dstChainId parameter. As a result, it permits requests with arbitrary and possibly invalid destination chain IDs, which could lead to incorrect increment of the traceID in the contract's storage for invalid requests.

  function _sigReq(SignTypes.SigReqParams memory params) internal virtual sigFee(params.payload) whenNotPaused {
    SignStorage storage $ = _getSignStorage();
    _validatePublicKey(_getWalletByIndex(params.walletTypeId, params.owner, params.walletIndex));
    bytes32 walletId = keccak256(abi.encode(params.walletTypeId, params.owner, params.walletIndex));
    unchecked {
      $._traceId = $._traceId + 1;
    }
    emit ZrSigRequest(
      $._traceId,
      walletId,
      params.walletTypeId,
      params.owner,
      params.walletIndex,
      params.dstChainId,
      params.payload,
      params.isHashDataTx,
      params.broadcast
    );
  }

Consider adding a check to the _sigReq function to verify that the dstChainId parameter is a valid chain ID.

Remediation Plan

RISK ACCEPTED: The ZenRock Labs team made a business decision to accept the risk of this finding and not alter the contracts, asserting that: The function in question is accessible via an external function that has a modifier to perform the necessary checks, ensuring controlled and safe usage.

The _zrKeyRes() function, triggered by the external zrKeyRes() function, is responsible for processing key response requests. This process includes validating the authenticity and integrity of the response through signature verification and updating the contract's state, specifically the wallets mapping. However, this function lacks a check to prevent execution when the contract is in a paused state, which could allow ongoing operations that modify the contract's state.

Similarly, the _sigRes() function, triggered by the external zrSigRes() function, is responsible for finalizing the signature response by emitting an event. This function also lacks a check to prevent execution when the contract is in a paused state, which could allow ongoing operations.

Here, the ZrKeyRes function is called when the contract is paused, succesfully updating contract's public keys in storage:

  function test_ResponseFunctionsWhenContractIsPaused(string memory publicKey) public {
    test_pause();
    assertTrue(zrSign.paused());
    string[] memory keys = zrSign.getZrKeys(_getWalletTypeId(), MPC);
    uint256 keysLength = keys.length;
    assertEq(keysLength, 0);
    test_ZrKeyRes(publicKey);
    keys = zrSign.getZrKeys(_getWalletTypeId(), MPC);
    keysLength = keys.length;
    assertEq(keysLength, 1);
    assertTrue(zrSign.paused());
  }
}

 function test_ZrKeyRes(string memory publicKey) public {
   if (bytes(publicKey).length < 5) publicKey = string.concat(publicKey, "abcde");
   test_WalletTypeIdConfigByOwner(); // make the wallet type Id supported (44-60)
    
   (address signer, uint256 signerPrivateKey) = makeAddrAndKey("MPC"); // Get private key to sign locally
   bytes32 dataToSign = _getSignedPayload(_getWalletTypeId(), signer, 0, publicKey); // Encode data to sign
   (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, dataToSign); // Sign the data
   bytes memory signature = abi.encodePacked(r, s, v); // Get the signature
    
   vm.startPrank(MPC);
   SignTypes.ZrKeyResParams memory zrKeyResParams = SignTypes.ZrKeyResParams({
     walletTypeId: _getWalletTypeId(),
     owner: MPC,
     walletIndex: 0,
     publicKey: publicKey,
     authSignature: signature
   });
    
   zrSign.zrKeyRes(zrKeyResParams);
   string[] memory keys = zrSign.getZrKeys(_getWalletTypeId(), MPC);
   uint256 keysLength = keys.length;
   assertEq(keysLength, 1);
 }

Consider adding the whenNotPaused modifier to the aforementioned functions to prevent them from being executed when the contract is in a paused state.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation and adding the whenNotPaused modifier to the _zrKeyRes() and the _sigRes functions.

The zrKeyRes() and zrSignRes() functions, which can be called by any address, utilize the _mustValidateAuthSignature() function for validating data signatures. Currently, _mustValidateAuthSignature() does not verify whether the authAddress recovered from the signature is the same as the msg.sender of the transaction. This lack of verification could potentially allow an unauthorized user to execute these functions and pass validation checks on behalf of another address.

Consider adding a check to the _mustValidateAuthSignature() function to verify that the authAddress recovered is the msg.sender of the transaction.

Remediation Plan

RISK ACCEPTED: The ZenRock Labs team made a business decision to accept the risk of this finding and not alter the contracts, asserting that: This behavior is intentional, allowing anyone with the correct signature and data to pay for L1 gas fees.

The _sigRes() function, accessible via the external zrSignRes() function, is tasked with finalizing signature responses and emitting an event. However, it does not validate the traceId to confirm that it is neither previously resolved nor nonexistent. This lack of validation can lead to the same traceId being emitted multiple times or the use of invalid traceIds, potentially compromising data integrity.

Consider introducing a validation mechanism to ensure that the traceId is legitimate. This will prevent duplicate or invalid traceIds from compromising the integrity of the transaction records.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 5ac1dfc by following the mentioned recommendation.

The _sigRes() function, accessible via the external zrSignRes() function, is responsible for finalizing the signature response does not have a verification mechanism to verify that the function input data has been executed previously and emitted as resolved.

This oversight leads to redundant events and opens the door for potential spamming, as malicious actors could exploit this to flood the network with repetitive transactions.

Here, the same data is emitted in different transactions:

  function test_ZrSignResRepeatedData(
    string memory chainId,
    string memory publicKey,
    uint256 _newBaseFee,
    uint256 _newNetworkFee
  ) public {
    test_ZrSignTx(chainId, publicKey, _newBaseFee, _newNetworkFee);
    (address signer, uint256 signerPrivateKey) = makeAddrAndKey("MPC"); // Get private key to sign locally
    bytes32 dataToSign = _getSignedPayload(_getWalletTypeId(), signer, 0, publicKey); // Encode data to sign
    (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, dataToSign); // Sign the data
    bytes memory signature = abi.encodePacked(r, s, v); // Get the signature

    bytes memory payload = abi.encode(SRC_CHAIN_ID, 2, signature, true);
    bytes32 newDataToSign = keccak256(payload).toEthSignedMessageHash();
    (v, r, s) = vm.sign(signerPrivateKey, newDataToSign); // Sign the data
    bytes memory newSignature = abi.encodePacked(r, s, v); // Get the signature

    SignTypes.SignResParams memory signResParams = SignTypes.SignResParams({
      traceId: 2,
      signature: signature,
      broadcast: true,
      authSignature: newSignature
    });

    for (uint i; i < 10; ++i) {
      vm.expectEmit(address(zrSign));
      emit ZrSigResolve(2, signature, true);
      zrSign.zrSignRes(signResParams);
    }
  }

Consider adding a check to the zrSignRes function or the _sigRes() function to not allow for data that has been executed previously and emitted as resolved.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation and adding a check to verify that the data has not been previously processed.

The Sign.sol contract contains the _validatePublicKey function which validates a public key by checking for a minimum key length of 4 bytes. This threshold is substantially below the length required for secure public keys on blockchain technologies like Bitcoin and Ethereum. In Bitcoin, for example, compressed public keys are 33 bytes, and uncompressed keys are 65 bytes. The inadequacy of the check may permit the use of invalid or insecure public keys, undermining the security of the contract and potentially exposing it to unexpected risks.

Enforce stricter validation that aligns with accepted cryptographic standards. Specifically, is it recommended updating the _validatePublicKey function by requiring that the minimum length is greater than 32 bytes.

For Ethereum addresses, which are derived from the rightmost 160 bits of the Keccak-256 hash of the public key, an additional check can ensure the address falls within the appropriate range:

bytes memory pubKeyBytes = bytes(publicKey);

if (uint256(bytes32(pubKeyBytes)) > type(uint160).max) {
  revert InvalidEthereumAddress(ownerBytes);
}

Remediation Plan

RISK ACCEPTED: The ZenRock Labs team made a business decision to accept the risk of this finding and not alter the contracts, asserting that: Our contract is designed to accommodate a wide range of wallet types, including BTC addresses, EVM addresses, and Cosmos wallets.

The Sign.sol contract inherits from a local version of OpenZeppelin upgradeable contracts. These contracts contain code that pertains to the Upgradeable version of themselves, signaling a name modification removing the Upgradeable suffix added to upgradeable contracts.

The affected contracts are:

- AccessControl in "../contracts/AccessControl.sol"

- Context in "../contracts/Context.sol"

- Pausable in "../contracts/Pausable.sol"

It is recommended to not modify OpenZeppelin's original codebase, including naming conventions. Maintain the conventional naming pattern for OpenZeppelin's upgradeable contracts:contractNameUpgradeable.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation.

Currently, the Sign contract does not follow the NatSpec tag specification that should be annotated to the contract's struct for ERC-7201. According to ERC-7201:

A namespace in a contract should be implemented as a struct type. These structs should be annotated with the NatSpec tag @custom:storage-location <FORMULA_ID>:<NAMESPACE_ID>, where <FORMULA_ID> identifies a formula used to compute the storage location where the namespace is rooted, based on the namespace id.

Consider adding the following namespace tag to properly follow the ERC-7201 specification:

/// @custom:storage-location erc7201:zrsign.storage.sign

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation.

The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail.

Make sure to specify the target EVM version when using Solidity 0.8.20, especially if deploying to L2 chains that may not support the PUSH0 opcode. Stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation, and locking the contracts to compiler version 0.8.19.

The ZrSign contract contains the withdrawFees. This function allows an address with the TOKENOMICS_ROLE to withdraw all funds from the contract. It does so by calling the transfer function.

The transfer() and send() functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks, which may break already deployed contract systems that make fixed assumptions about gas costs. For example, EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction.

Avoid the use of transfer() and send() and do not otherwise specify a fixed amount of gas when performing calls. Use .call{value: _value}("") instead. Use the checks-effects-interactions pattern and/or reentrancy locks to prevent reentrancy attacks to the system when executing these calls. For more reference, see https://swcregistry.io/docs/SWC-134/.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation.

The SRC_WALLET_TYPE_ID constant declared in Sign.sol is not being used throughout the contract. This may indicate missing or unfinished implementations.

Assess if the constant was intended for features that have not yet been implemented. If so, complete these implementations to utilize the constant effectively. If the SRC_WALLET_TYPE_ID constant is not planned for use in present or upcoming features, consider removing it.

Remediation Plan

ACKNOWLEDGED: The ZenRock Labs team made a business decision to acknowledge this finding and not alter the contracts, asserting: We recognize this and choose to retain the constant for future utility.

The Sign.sol contract currently defines the getTraceId() and isWalletTypeSupported() functions with public visibility, even though they are not called from within the smart contract, resulting in higher gas costs than necessary.

Modify the aforementioned functions with the external visibility modifier.

Remediation Plan

ACKNOWLEDGED: The ZenRock Labs team made a business decision to acknowledge this finding and not alter the contracts, asserting: We acknowledge this issue. Although these functions are currently not utilized, we intend to keep them available for potential future use.

The ZrSign contract’s chainIdConfig function allows the ADMIN to set the chain ID for specific wallet types, similar to walletTypeIdConfig which configures support for specific wallet types based on their purpose and coin type.

However, both functions lack validation for their inputs. This oversight could permit the assignment of invalid or unexpected values, such as an empty string for the chainId or 0 values for the walletTypeId, purpose or coinType input values, potentially compromising the contract's informational integrity.

Verify that the aforementioned input values provided are non-empty or valid.

Remediation Plan

ACKNOWLEDGED: The ZenRock Labs team made a business decision to acknowledge this finding and not alter the contracts, asserting: We acknowledge this issue and are considering appropriate modifications to enhance verification processes without compromising functionality

In the current implementation of the withdrawFees() function within the ZrSign contract, the use of the payable modifier is unnecessary and potentially misleading. The payable modifier in Solidity is used to allow a function to receive native assets along with a call. However, withdrawFees() is designed to send native assets (the collected fees) from the contract to the function caller, and thus should not accept incoming assets transactions.

Remove the payable modifier from the withdrawFees() function to correctly reflect its intended use and to prevent erroneous transfers to the function.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation.

The function uppause() in the ZrSign contract appears to be misnamed, as its action is to unpause the contract.

Rename uppause() to unpause() to accurately reflect its purpose.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation.

The _mustValidateAuthSignature function reverts with the UnauthorizedCaller error if the authAddress recovered from the data and signature provided does not have the MPC_ROLE. However, the error name is misleading, as it assumes that the caller of the transaction is the authAddress recovered, which may not always be the case.

Consider renaming the error to a name that accurately describes the issue.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation.

The _zrKeyRes() function emits the ZrKeyResolve event and calculates the walletIndex by unnecessarily reading from storage: wallets[id].length - 1. This approach is inefficient as it incurs higher gas costs.

Avoid direct storage access for the walletIndex and use the params.walletIndex value instead to improve efficiency and reduce gas costs.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation.

The Sign contract employs a transparent upgradeable pattern and initiates the Pausable_init_unchained() and Sign_init_unchained() within the _Sign_init() function during the contract's initialization. However, it fails to call the _AccessControl_init() or the _AccessControl_init_unchained() function. While this oversight does not impact the contract's functionality, owing to the currently empty implementation of this initialization function, it diverges from best practices associated with using the AccessControlUpgradeable module.

Add the _AccessControl_init_unchained() function call to the __Sign_init() function to align with standard practices for initializing inherited AccessControl mechanisms in the Sign contract.

Remediation Plan

SOLVED: The ZenRock Labs team has addressed the finding in commit 4492c49 by following the mentioned recommendation.