Security Audit
January 1, 2024
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
Quark is an Ethereum smart contract wallet system designed to run custom code with each transaction.
The following source code was reviewed during the audit:
4910dadbdfa078eda1011cf51a6b8cbdbfff08ea
d67860fa8fc4e94cf54d3ee6931ca52c5d4ea07d
Specifically, we audited the following contracts.
Contract | SHA256 |
---|---|
codejar/src/CodeJar.sol |
|
codejar/src/CodeJarStub.sol |
|
quark-core/src/QuarkScript.sol |
|
quark-core/src/QuarkStateManager.sol |
|
quark-core/src/QuarkWallet.sol |
|
quark-core/src/periphery/BatchExecutor.sol |
|
quark-core-scripts/src/Ethcall.sol |
|
quark-core-scripts/src/Multicall.sol |
|
quark-core-scripts/src/UniswapFlashLoan.sol |
|
quark-core-scripts/src/UniswapFlashSwapExactOut.sol |
|
quark-core-scripts/src/lib/UniswapFactoryAddress.sol |
|
quark-core-scripts/src/vendor/uniswap_v3_periphery/PoolAddress.sol |
|
quark-proxy/src/QuarkMinimalProxy.sol |
|
quark-proxy/src/QuarkWalletProxyFactory.sol |
|
terminal-scripts/src/TerminalScript.sol |
|
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.
Click on an issue to jump to it, or scroll down to see them all.
block.timestamp
in Uniswap operations
nextNonce()
does not return uint96
max value as a valid nonce
nextNonce()
walletAddress
CodeInvalid
in CodeJar
We quantify issues in three parts:
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. |
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
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
isValidSignature
implementation as an example.Fixed by requiring messages to be encoded with a wallet-specific domain separator.
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:
Remediations to Consider
msg.value
as part of QuarkWallet operation signature, ormsg.value
to be transferred as part of the operation but instead be taken from the contract balance, andWe decided that there are no real use-cases where executeQuarkOperation and executeScript needs to be payable, so we changed these to be non-payable.
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
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
block.timestamp
in Uniswap operations
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:
deadline
parameter to all functions that perform a swap on the user's behalf, such as swapAssetExactIn()
and swapAssetExactOut()
.Changed to allow user to pass deadline in Uniswap params.
nextNonce()
does not return uint96
max value as a valid nonce
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.
Additional update in https://github.com/compound-finance/quark/commit/3b64552892d7df3a367087b3bef98f35735e1611.
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.
Most scripts won’t need a start timestamp. Scripts that require one can implement a check themselves.
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.
walletStorage
is now public so it can be read directly.
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:
nonceScriptAddress
is setWe added an event for clearNonce to track replayable txns. We don’t believe the other events are needed.
nextNonce()
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:
nextNonce()
to accept an offset argument to facilitate skipping the previously utilized range of noncesThis is expected as we only intend to run this function off-chain. We slightly optimized it without changing the function definition.
walletAddress
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.
This was done on purpose. walletAddress is unique so there is no need to index it.
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()
CodeInvalid
in CodeJar
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.
After re-architecting the contracts to use a minimal proxy, this optimization no longer exists.
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.