Static Analysis
The following are the remaining high/medium/low risk warnings and why we chose to accept these risks.
This ignores a few pedantic cases, such as mock contracts used for testing, and warnings from OpenZeppelin dependencies.
High Risk
VaultUpgradeable - Arbitrary “from” in transferFrom
Reason:
from is not msg.sender and instead comes from a parameter. This means any caller with access can call depositERC20 to drain a users funds who previously approved the vault
Why it’s okay: The function in question is only callable by Escrow, who gets these values from markets.
These are trusted registered contracts which are included in the audit, and whose upgradability requires UpgradeGatekeeper to register the implementation contract.
This means the risk is Colle registering a malicious market, or a malicious upgrade occurring
Colle registering a malicious contract will be mitigated by offloading the ability to register/unregister markets to a multisig and eventually a DAO
Malicious upgrades are mitigated by requiring upgrades be approved by the UpgradeGatekeeper contract , which is also owned by a multisig but eventually will be by that same DAO
TeamSmartWallet - Sends ETH to arbitrary user
Reason:
Contract funds can be drained by these actions. The functions in question are executeRawTransaction, transferNative and permitTransferNative
Why it’s okay:
Arbitrary users cannot call these functions, they are guarded by the appropriate roles, and these functions must exist for this smart contract to act as a wallet for the team.
As the team owner will have the DEFAULT_ADMIN_ROLE role, only they can grant the roles that can lead to these risks.
The only external attack vector is the upgradability of this contract. This is mitigated by requiring DEFAULT_ADMIN_ROLE consent to any upgrade, giving the user full control.
Medium Risk
EscrowUpgradeable - Uses dangerous strict equality
Reason:
sender and saleId as a arguments for _updateSaleTo[State] and _payout internal functions could be manipulated by the caller to bypass reverts
Why it’s okay:
These functions are internal, and only called by _updateSaleState, therefore indirectly from updateSaleState and permitUpdateSaleState.
The sender and saleId (sale struct) are directly passed through by updateSaleState and permitUpdateSaleState. Sender is either msg.sender or the EIP-712's signer, and the sale id is passed in directly as a argument and unmodified by _updateSaleState.
We guard that the sender must either be the buyer associated with that sale struct for buyer action, the seller associated with that sale struct, or Colle. These functions further constrian themselves to which of those three actors is allowed to make each individual state changed.
Therefore, despite the caller of these parameters being able to choose the parameters they see fit, everything is managed by the contract.
TeamSmartWallet - Ignores return value by IERC.approve
Reason:
The buy and updateSale functions may call IERC20.approve internally, but ignore the return value.
Why it's okay:
These functions are helpers for teams, with approvals added in a effort to minimize transaction signatures required when making purchases. However, they have the ability to approve separately from these functions, so a failure with approval inside the contract does not have to prevent them from attempting the actions all together.
Low Risk
MarketHub - Has external calls inside a loop
Reason:
handleTokenIdNoLongerAvailable loops over all registered markets and calls a function. This is a indeterministic amount of work that could scale out of control if too many markets get registered
Why it’s okay:
There will be a handful of markets registered. The number of markets in the list is controlled. On launch, this will start as a size of two, and there are plans to add a third market in the future. It will not, however, grow out of control.
TeamSmartWalletFactory - Reentrancy attack vector
Reason:
Events get emitted after proxy deployment. The proxy deployment could in its initialize logic perform a reentrancy attack on the factory, affecting the ordering of events emitted to not represent reality
Why it’s okay:
We must emit these events after proxy deployment, because the proxy address is what is being emitted. Colle controls these proxy implementations and the deployment of new proxies, so Colle has the control to guarantee that no reentrancy attack will occur.
EscrowUpgradeable - Uses timestamp for comparisons
Reason:
Timestamps can be manipulated by the block validators, so they should not be used for randomness or be assumed to have precision.
Why it’s okay:
Our usage is over hours, defaulting to 24 and 48 hours respectfully for their use cases.
There is no randomness generated from these timestamps, and we accept it's okay if they are off by a few minutes from reality.
There is no MEV for miners to abuse these values, they are just waiting periods before an actor can dispute a sale or deposit funds.
Different version of Solidity are used
Reason:
There may be discrepancies between Solidity versions. Ideally our dependencies use the same version as us
Why it’s okay:
We use the same major/minor version between our contracts and our dependencies. The only difference is in patch version. We trust no breaking changes occurred between these patch versions.
EscrowUpgradeable - High cyclomatic complexity
Reason:
We have 11 possible states in the state machine that can be transitioned into, which created a long if/else chain that is complex
Why it’s okay:
The alternative would be to use a switch statement, which Solidity does not support, or a mapping of enum values to functions to delegatecall, which would increase gas usage.
The if/else chain is on a enum that is passed in through the transaction parameter, so we need this is both human readeable and straight forward, despite it's high cyclomatic complexity.
We also believe we have adequate unit tests to ensure the correctness of the code.
Last updated