Compound Quark A-1

Note: Also independently discovered and fixed by the team during the audit

In the structHash encoded to verify operations inside executeQuarkOperation, the dynamic parameters scriptSource and scriptCalldata should be hashed according to EIP712 specifications.

Due to the above non-compliance, signatures generated by 3rd party systems that follow EIP712 specification will not be valid by the QuarkWallet verification.

Remediations to Consider

• Consider keccak256 hashing op.scriptSource and op.scriptCalldata before encoding obtained hashes into the structHash.

The current EIP1271 signature validation scheme for EOA owners verifies that the provided hash/signature pair matches the signer of each Quark wallet. However, the signature verification does not include the intended Wallet address to be used.

For all Wallets owned by the same signer this validation will return the same hash/signature pair as valid. If all conditions are met, this could lead to signed transactions being replayed across different wallets.

For example, if the external protocol signed for does not include the message digest and if conditions such as token balances and approvals are met, they could be replayed for each QuarkWallet that belongs to the same owner.

Remediations to Consider

• Include the wallet’s address in the message digest with a domain separator and recover the signer afterward. See Safe’s isValidSignature implementation as an example.

In the QuarkWallet contract, anyone with a valid signature can call the executeQuarkOperation() function. A valid signature can relatively easily be obtained from pending user transactions in the mempool. However, the transaction value is not part of the signature and can be freely set.

When QuarkWallet operation execution simply relies on the provided transaction value to perform the operation and set nonce, it may be susceptible to griefing attack.

A malicious attacker may grieve an unsuspecting user by copying their transaction while providing infinitesimal amounts of native assets, e.g. 1 wei. Unless there are minimum amount requirements within the executed script, the operation will succeed and it will set nonce. As a result, the user’s pending transaction with the appropriate amount will fail as the provided nonce is invalid.

The user will need to submit a new transaction with a new nonce which can also be griefed by the attacker in the same way.

Potentially affected operations:

• EthCall.run()
• TransferActions.transferNativeToken() in TerminalScript

Remediations to Consider

• Include msg.value as part of QuarkWallet operation signature, or
• Do not rely on msg.value to be transferred as part of the operation but instead be taken from the contract balance, and
• Document known risks for developers of QuarkScripts.

For QuarkWallet, a signed Quark operation executing a script logic can enable that specific nonce to be reusable by clearing the nonce within the script execution. If the signer wants to execute the same script with a different calldata payload it needs to sign a new QuarkWallet operation with a different calldata for the same nonce.

However, both signatures (for the original and updated calldata) can be executed and will be valid until the nonce is disabled. This could allow a malicious attacker to front-run and execute undesired operations on behalf of the user, due to old signatures targeting reusable script logic being still valid.

Remediations to Consider

• Consider allowing wallet signers to invalidate previous signatures, or
• Prevent updates to calldata for replayable QuarkWallet operations, or
• Document known risks for developers of QuarkScripts and users.

In the TerminalScript.sol, UniswapSwapActions.swapAssetExactIn() and UniswapSwapActions.swapAssetExactOut() functions feature calls to uniswapRouter with deadline/expiration parameter hardcoded to block.timestamp value. These functions do not allow users to configure a deadline for their underlying swaps on Uniswap. As a consequence, this missing feature enables pending transactions to be maliciously executed at a later point.

Hardcoded deadline value of block.timestamp makes it possible for the transaction to be executed long after its expected execution time (up to the expiration time in the signature of the QuarkOperation). For example, it may remain in the mempool for extended periods until it becomes interesting for block builders to include it. Also, it can be intentionally delayed to extract the maximum slippage value.

Uniswap offers deadline/expiration parameters in their functions as a protection mechanism. Hardcoding these parameters to block.timestamp circumvents this protection mechanism and makes this integration layer built upon Uniswap less secure for end users.

Remediations to Consider:

• Introduce a configurable deadline parameter to all functions that perform a swap on the user's behalf, such as swapAssetExactIn() and swapAssetExactOut().

In the QuarkStateManager, nextNonce() does not return the max uint96 value as a valid nonce even though it is valid by the specification. This is due to an incorrect condition in the for loop which should be <= instead of <.

function nextNonce(address wallet) external view returns (uint96) {
    for (uint96 i = 0; i < type(uint96).max; i++) {
        if (!isNonceSet(wallet, i) && (nonceScriptAddress[wallet][i] == address(0))) {
            return i;
        }
    }
    revert NoUnusedNonces();
}

Note: one important side effect of making a suggested change to the loop condition is that the statement revert NoUnusedNonce() will become unreachable, since i++ would in an edge case, when even max uint96 value is not a valid nonce, overflow and revert before reaching the revert expression.

Remediations to Consider:

Consider updating nextNonce() implementation to return valid nonces from the specified range or update specification to match the current implementation.

The current QuarkOperation struct contains an expiry timestamp that allows wallet signers to specify the time until its signed operation is valid. However, the operation payload has no start timestamp validity. To allow users and signers to have more granular control over signed operations and their execution, consider providing time for the start of the timestamp validity of an operation. This could allow use cases such as signing operations that will be executable in a specific time range.

QuarkStateManager contract’s read() and write() functions are only accessible if there is an active nonce for the specific wallet context. To access values in walletStorage outside of script execution, the storage slots of each wallet inside QuarkStateManager need to be pre-calculated and read through external methods. This is inconvenient and hinders 3rd party tools from integrating with QuarkWallet.

To improve integration capabilities, consider adding a set of view functions to QuarkWallet to remove the need for 3rd parties to interact with the QuarkStateManager contract for retrieving the QuarkWallet-related state.

In QuarkStateManager, nonce updates indicate important state changes. When nonce is set, it commonly represents that the associated QuarkWallet operation has been successfully performed and that it cannot be executed anymore. If the nonce is cleared, it means that the associated QuarkWallet operation is replayable and can be performed multiple times.

Since QuarkWallet represents an infrastructure component that many will be integrating with, it is important to enable easier off-chain tracking and monitoring.

Consider adding events for the following actions to facilitate future integrations:

• when the nonce is set
• when the nonce is cleared
• when the nonceScriptAddress is set

In QuarkStateManager, nextNonce() implementation on each invocation requires iteration from the start (nonce 0) in order to determine the next valid nonce. However, this is inefficient and potentially problematic when a particular instance of QuarkWallet has already used many of the nonces, and determining the next valid nonce would consume a lot of gas.

To improve integration capabilities, consider:

• facilitating off-chain tracking of used nonces with emitted events and/or
• updating nextNonce() to accept an offset argument to facilitate skipping the previously utilized range of nonces

In QuarkWalletFactory, the QuarkWalletDeployed event is emitted when a new QuarkWallet is created. Signer and executor parameters for this event are indexed. However, walletAddress parameter is not. Consider indexing the walletAddress parameter for easier off-chain tracking and monitoring.

In TerminalScript, multiple functions that accept array arguments do not perform validation that these arguments have matching length.

• CometSupplyActions.supplyMultipleAssets()
• CometWithdrawActions.withdrawMultipleAssets()
• CometSupplyMultipleAssetsAndBorrow.run()
• CometRepayAndWithdrawMultipleAssets.run()

In CodeJar contract, a CodeInvalid error declaration is present. However, this error is not used at all. Consider removing it.

Mapping variables in QuarkStateManager use comments to indicate the intention of variables within nested mappings. However, named parameters in mapping types are supported from the 0.8.18 version of Solidity. Consider replacing comments in mappings with named parameters.

• The result will always be returned here:

  // otherwise, return the result.
  return result;
  

  Reference: QuarkStateManager.sol#L164-165

• Nonce 0 is valid:

  /**
   ...
   *** @dev `0` is not a valid nonce**
   ...
   */
  

  Reference: QuarkStateManager.sol#L42-48

  /**
   * @notice Returns the next valid unset nonce for a given wallet (note that 0 is not a valid nonce)
   ...
   */
  

  Reference: QuarkStateManager.sol#L59-65

• There is a typo in the getInitCode() note:

  // Note: The gas cost in memory is `O(a^2)`, thus for an array to be
  //       more than 2^32 bytes long, the gas cost would be (2^32)^2 or
  //       about 13 orders of magnitude above the current block gas
  //       limit. As such, we check the type-conversion, but understand
  //       it is not possible to accept a value **whose length whose length**
  //       would not actually fit in 32-bits.
  

  Reference: CodeJar.sol#L70

In the QuarkWallet, whenever executeQuarkOperation() is performed domainSeparator is calculated and used within signature verification functionality.

bytes32 domainSeparator = keccak256(
  abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(NAME)), keccak256(bytes(VERSION)), block.chainid, address(this))
);

However, as can be seen in the snippet above, the value of domainSeparator does not depend on the executeQuarkOperation() arguments. As a matter of fact, the domainSeparator value will remain constant for the whole lifetime of the contract unless the chain is forked and block.chainid obtains new value.

Consider precalculating and caching the domainSeparator value in the QuarkWallet constructor to remove unnecessary operations. In executeQuarkOperation(), reuse the domainSeparator value and recalculate it only if block.chainid changes.