CodeHawks Escrow Contract - Competition Details

Note to Judge: Think of this as a combination of a QA, Gas and Analysis report from code4rena's submission standards

1. Summary of Protocol

The escrow contract is used for protocols looking for private audits on the codeHawks platform. It act as a agreement between the protocol and auditor where protocol will first lock funds in the Escrow conrtact and pay auditor once audits are completed.

2. Actors

There are 3 actors in the project, buyers, sellers and arbiter. The details can be viewed here

3. Analysis of codebase

• confirmedReceipt(): Primary exit point of contract. Buyer pays seller the agreed price for the audit completed. This function can be called only be called by the buyer

• initiateDispute(): Buyer or Seller can initiate dispute anytime if there are disagreements, but requires a arbiter set by buyer

• resolveDispute(): Secondary exit point of contract. If a dispute is initiated via initiateDispute(), only arbiter can call this function to resolve dispute for a fee paid by buyer and return/pay funds to buyer/seller

4. Centralization Risks

• resolveDispute(): If a dispute is initiated, it cannot be cancelled, and requires arbiter to retrieve funds for buyer. Arbiters can then ultimately decide the amount of funds to send to buyer (protocol) and seller (auditor).

• confirmReceipt(): If no arbiter is set, the outflow of funds ultimately depends on this function. The interesting thing here is that based on the design of the escrow contract, buyer cannot purposely withold funds if no arbiter is set as this function is the only way to transfer funds out and the only recipient is the seller. If not, buyer funds transferred is locked forever in escrow contract.

5. Architecture Improvements

5.1 Consider only allowing buyer to use whitelisted ERC20 tokens instead of any token

Impact and Recommendation

Consider implementing a blacklist of malicious tokens or only whitelist accepted tokens such as USDC, USDT and trusted project tokens. This also protects against tokens with other caveats such as rebasing and fee-on-transfer tokens that can affect seller's and arbiter's payment.

5.2 Consider not allowing immediate initiation of dispute

Impact

Escrow.sol#L102-L106

Currently, the inititateDispute() function allows dispute to be initiated at anytime by buyer or seller. This allows seller to immediately call the function to force buyer to delegate escrow to arbiter and force the payment of i_arbiterFee if it exists, even if not desired by buyer.

Recommendation

Consider only allowing the initiation of dispute after a delay. This delay could represent the duration of the audit performed.

5.3 Consider adding support for multiple sellers (auditors)

Impact

Consider supporting multiple auditors (sellers) so that a single escrow contract can delegate to multiple auditors without the need for re-deployment of escrow contract. An example use-case could be a private audit/audit-competition with different, non-affliated auditors.

Recommendation

Consider adding an additional onlyBuyers function that allows confirming recipt and sending of funds to multiple sellers. An state variable of an array of addresses representing sellers can be assigned in the constructor but cannot be altered any more once assigned.

5.4 Consider using a stricter check in constructor when assigning i_price

Impact

Escrow.sol#L43

Lack of emergency withdraw option would mean in the event that buyer mistakenly sends more tokens then expected, best case it will require arbiter to retrieve the funds for them. Worse case, if no arbiter is set, funds are locked forever unless buyer calls confirmReceipt() to send funds to seller.

Recommendation

Use a stricter check in the constructor

6. Gas Improvement Findings

6.1 Cache state variable i_seller and i_tokenContract in confirmReceipt()

Escrow.sol#L94-L99

Caching i_seller and i_tokenContract saves SLOADs when loading for event emission and safeTransfer().