Exploring Missed Vulnerabilities: CodeHawks Escrow Audit Contest

This marks the start of a new blog series dedicated to my participation in audit contests. As a reader, your feedback is highly valued, so please feel free to share your thoughts with me.

Introduction

CodeHawks is an emerging public auditing platform tailored for Web3 security auditors, and at its core, is none other than the OG Patrick Collins himself.

The Contest Summary comprises four Medium-risk, three Low-risk, and 39 Gas/Info findings. My primary focus will be on the Medium and Low-risk categories, although I will also highlight noteworthy Gas/Info issues. For each finding, I will delve into its root cause and share the lessons learned to ensure that similar oversights are not repeated in future audits.

The Findings

M-02. Lack of emergency withdraw function when no arbiter is set

The Vulnerability: If there is no designated arbiter, the buyer is left with no means to retract the funds sent to escrow, potentially resulting in the permanent loss of tokens. The ideal scenario involves the active involvement of an arbiter. This process unfolds as follows: Initially, the buyer or seller can call the initiateDispute() function to signal a potential dispute. Subsequently, the arbiter can step in and address the dispute by invoking the resolveDispute() function.

However, in cases where no arbiter is assigned, and the buyer is dissatisfied with the seller’s delivery, the only recourse to release the funds from escrow is for the buyer to execute the confirmReceipt() function, effectively transferring the funds to the seller.

Learning: It’s essential not to assume that something will work correctly solely because the documentation states so. In my case, I relied on the documentation, which indicated that the arbiter could be set to address(0). This led me to believe that there wouldn’t be any issues in the workflow.

M-01. Fee-on-transfer tokens aren’t supported

The Vulnerability: Fee-on-transfer tokens are not supported by the current escrow implementation. These tokens, as the name suggests, incur a small fee with each transfer. Consequently, the actual amount transferred to the Escrow address will be less than the stated price. If the contract’s balance falls below the required threshold, the code will revert with an error labeled Escrow__MustDeployWithTokenBalance.

M-03. Funds can be lost if any participant is blacklisted

The Vulnerability: in Escrow.sol, if any of the participants is blacklisted(Popular Stable Coins like USDT and USDC have these functionalities) , the funds for every participant will be locked permanently.

M-04. Fixed i_arbiterFee can prevent payment

The Vulnerability: i_arbiterFee is a fixed value and can brick payment in resolution of disputes if the payment token has a rebasing balance. in case i_tokenContract is a token that has a rebasing balance. i_arbiterFee can be bigger then the current balance and resolveDispute will revert in the following error Escrow__TotalFeeExceedsBalance

Learning: The learnings from the above three issues delve with the various types of ERC20 tokens in addition to standard ERC20 tokens, it’s essential to verify a protocol’s compatibility with various token types, including Fee-On-Transfer tokens, tokens with blocklists, and pausable tokens. Thoroughly assessing these aspects can prevent potential issues during implementation.

Additional details regarding ERC20 token variants can be found here.

L-01. Constructor of Escrow should make sure that buyer, seller, arbiter are different from each other.

The Vulnerability: As arbiter should be impartial, when buyer or seller is same with arbiter, there would be unfair arbitration.

Learning: I reported the issue during the contest, My point of view was basically, can buyer, seller, arbiter be set to same address.

L-02. If the arbiter is not set, arbiter fee should not be positive

The Vulnerability: If the arbiter is not set, there is no need to set arbiter fee as well.

Learning: Once again, I relied on the documentation assuming happy paths. Don’t read the documentation too early !!!

L-03. Lack of proper event emission at resolveDispute function.

The Vulnerability: The resolveDispute function’s event emits the buyer and seller addresses. However, it does not include any other information.

Learning: Make sure events are emitted for critical/important functions and make sure all the necessary parameters are part of it

Gas Optimizations / Informationals

1. Use Openzeppelin Minimal Clones to Save a Lot of Gas — Clones are small and inexpensive smart contracts that delegatecall’s to an implementation contract, eliminating the need to deploy the entire contract repeatedly.

2. Switch OZ to Solmate — Solmate’s implementation of the libraries used on the protocol are more optimized saving gas to the user.

3. The nonReentrant modifier should occur before all other modifiers — This is a best-practice to protect against reentrancy in other modifiers.

CodeHawks Escrow Audit Report — https://www.codehawks.com/report/cljyfxlc40003jq082s0wemya

Thank For Reading.

Connect with me: https://linktr.ee/zuhaib44