CodeHawks Escrow Contract - Competition Details
Cyfrin
Foundry
40,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Missing zero address check may result in loss of funds

devtooligan

Summary

The arbiter address can be inadvertantly set to the zero address which would disable arbiter functionality for the contract.

Vulnerability Details

The i_arbiter address is passed in to the constructor to be set at time of creation. There is no check whether the address is zero because a value of zero is used to indicate whether or not an arbiter has been set for the escrow.

constructor(
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) {
if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
if (buyer == address(0)) revert Escrow__BuyerZeroAddress();
if (seller == address(0)) revert Escrow__SellerZeroAddress();
if (arbiterFee >= price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
i_price = price;
i_tokenContract = tokenContract;
i_buyer = buyer;
i_seller = seller;
i_arbiter = arbiter;

Impact

Low. The severity could potentially be high if buyer was depending on the arbiter because, for example, there was concern about the trustworthiness of the seller.

However, the difficulty of triggering this is high. There would need to be a front-end bug or a transaction with some poorly created calldata that unintentionally set the arbiter address to zero.

Because the difficulty is so high, the overall impact of this finding is low.

Tools Used

Slither.

Recommendations

Consider adding an additional constructor parameter such as disableArbiter. If this is not explicitly called with true, then include a zero address check for arbiter in the constructor.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.