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.
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.
M-02. Lack of emergency withdraw function when no arbiter is set
The Vulnerability: If there is no designated
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
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
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
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.
i_arbiterFeecan prevent payment
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
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
Escrowshould make sure that
arbiterare 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
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
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
- 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.
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