Reach out for an audit or to learn more about Macro
or Message on Telegram

Compound Quark A-1

Security Audit

January 1, 2024

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Compound's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from November 21, 2023 to November 28, 2023.

The purpose of this audit is to review the source code of certain Compound Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
High 1 - - 1
Medium 3 - - 3
Low 2 - - 2
Code Quality 9 - 1 8
Gas Optimization 1 - - 1

Compound was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Trust Model, Assumptions, and Accepted Risks (TMAAR)

Quark is an Ethereum smart contract wallet system designed to run custom code with each transaction.

Entities

Trust Model

General Assumptions

Accepted Risks

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts.

Contract SHA256
codejar/src/CodeJar.sol

22ae83188a7255d2ebe30dda6d9e3c46fb2f0901a0124eb16f620d2b917e9782

codejar/src/CodeJarStub.sol

66734e234dfa0b4fd1b4b70c7bcd9fc0d7a8e80a29fb2ae5d770418dd2ba4280

quark-core/src/QuarkScript.sol

fdb9f6c4ef2d10c6d63181557aba28c19fd4fb6599714772258f40bb37aa5989

quark-core/src/QuarkStateManager.sol

bd4f2db0a4f69d5812d5414a96bd57ea4d8bcff20ea51b4c9b232c981f44c1eb

quark-core/src/QuarkWallet.sol

dc1a831a4cc0fabe3134a0d5726550e0cf5e8735b0217297ab2722fe0b91a8ff

quark-core/src/periphery/BatchExecutor.sol

86a65a94ed5dd0d411733b9d31e6f59228d8db2bc16f01264830df6551e60f95

quark-core-scripts/src/Ethcall.sol

7f11e6ebf4ea32a9e845a68a56661c6319cac0001552d4e0c96a71544a9d6272

quark-core-scripts/src/Multicall.sol

4502300eeffa6534d9a6a1201ef5665f6305b6b7357cdcd564658ffb3d59d55f

quark-core-scripts/src/UniswapFlashLoan.sol

f896a2d9401ed4abb1cc5974a3d123bc5b42ccbcdfe2e1814d5472b14704331a

quark-core-scripts/src/UniswapFlashSwapExactOut.sol

07c275e0d2f004e8228c65794f3aa6c7ccae47ce6af3f6d7c93fa61950c86781

quark-core-scripts/src/lib/UniswapFactoryAddress.sol

570e490638d8ffc7c5307b445dd9b3bb2574b18dd2ae8373914d466d331b4ebb

quark-core-scripts/src/vendor/uniswap_v3_periphery/PoolAddress.sol

6ae36efb47db4d2e41f28a6d3c13e8d21cb434b6fad2c744a4712bc197b3664b

quark-proxy/src/QuarkMinimalProxy.sol

ced473f309ca01882484fbf9bd766fb1ff36af7b2bc13e69421cd0beb7375d18

quark-proxy/src/QuarkWalletProxyFactory.sol

922bd6f6c366983f06e528b633d07c95ab2c89a4c8088754fba4762e60cc1300

terminal-scripts/src/TerminalScript.sol

d896cde1e344aa5774603c4c0b94ce06f2d77382b69fad454a4a3aaa4a8b2d33

Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

H-1

EIP712 verification in QuarkWallet is incompatible with the specification

Topic
Signatures
Status
Impact
Spec Breaking
Likelihood
High

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.
M-1

EIP1271 signature can be replayed across wallets with the same EOA owner

Topic
Signatures
Status
Impact
Medium
Likelihood
Medium

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

Response by Compound

Fixed by requiring messages to be encoded with a wallet-specific domain separator.

M-2

Griefing QuarkWallet operations with small native ETH amounts

Topic
Griefing
Status
Addressed
Impact
Medium
Likelihood
Low

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.
Response by Compound

We decided that there are no real use-cases where executeQuarkOperation and executeScript needs to be payable, so we changed these to be non-payable.

M-3

Multiple signatures may be valid for reusable nonce

Topic
Frontrunning
Status
Addressed
Impact
Medium
Likelihood
Low

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.
Response by Compound

This is by design. Allowing for flexible calldata in replayable scripts increases the flexibility of such scripts. We added some documentation to explain this design choice and added example test cases as well.

Documentation update: https://github.com/compound-finance/quark/commit/5411f20d3768e85ead059651a34538d5dd32ea12

L-1

Usage of hardcoded block.timestamp in Uniswap operations

Topic
Protocol Design
Status
Impact
Medium
Likelihood
Low

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().
Response by Compound

Changed to allow user to pass deadline in Uniswap params.

L-2

nextNonce() does not return uint96 max value as a valid nonce

Topic
Spec
Status
Impact
Low
Likelihood
Low

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.

Q-1

Lack of start timestamp for signatures

Topic
Signatures
Status
Wont Do
Quality Impact
Low

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.

Response by Compound

Most scripts won’t need a start timestamp. Scripts that require one can implement a check themselves.

Q-2

Inadequate UX/DX for accessing QuarkWallet’s storage

Topic
Integration
Status
Quality Impact
High

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.

Response by Compound

walletStorage is now public so it can be read directly.

Q-3

Missing events for important variable updates

Topic
Integration
Status
Quality Impact
Medium

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
Response by Compound

We added an event for clearNonce to track replayable txns. We don’t believe the other events are needed.

Q-4

Inneficient retrieval of valid nextNonce()

Topic
Integration
Quality Impact
Low

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
Response by Compound

This is expected as we only intend to run this function off-chain. We slightly optimized it without changing the function definition.

Q-5

QuarkWalletDeployed event not indexing walletAddress

Topic
Events
Status
Addressed
Quality Impact
Low

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.

Response by Compound

This was done on purpose. walletAddress is unique so there is no need to index it.

Q-6

Lack of length validation in TerminalScript

Topic
Sanity checks
Status
Quality Impact
Low

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()
Q-7

Unused error CodeInvalid in CodeJar

Topic
Errors
Status
Quality Impact
Low

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

Q-8

Use named parameters in mapping types

Topic
Best practice
Status
Quality Impact
Low

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.

Q-9

Inaccurate comments

Topic
Documentation
Status
Quality Impact
Low
  • 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

G-1

Use cached domainSeparator in executeQuarkOperation()

Topic
Gas optimization
Status
Addressed
Gas Savings
Medium

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.

Response by Compound

After re-architecting the contracts to use a minimal proxy, this optimization no longer exists.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Compound team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.