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

Lack of the input validation to check whether or not the arbiter address would not be `address(0)`, which lead to that both the seller and buyer can not initiate a dispute

0xmuxyz

Summary

Within the Escrow#constructor(), there is no input validation to check whether or not the arbiter address would not be address(0).

If a seller is missing to assign an arbiter address into the arbiter parameter when the Escrow#constructor() would be called to create a new Escrow via the EscrowFactory#newEscrow(), both the seller and buyer can not initiate a dispute when they call the Escrow#initiateDispute() due to being reverted by the validation (at the Escrow.sol#L103) in the Escrow#initiateDispute() below:

if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter();

Vulnerability Details

When a new Escrow would be created, the Escrow#constructor() would be called via the EscrowFactory#newEscrow() like this:
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L24
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L45

Within the Escrow#constructor(), an arbiter would be assigned into the arbiter parameter and it would be assigned into the i_arbiter like this:
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L37
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L49

/// @dev Sets the Escrow transaction values for `price`, `tokenContract`, `buyer`, `seller`, `arbiter`, `arbiterFee`. All of
/// these values are immutable: they can only be set once during construction and reflect essential deal terms.
/// @dev Funds should be sent to this address prior to its deployment, via create2. The constructor checks that the tokens have
/// been sent to this address.
constructor(
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter, /// @audit
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; /// @audit
i_arbiterFee = arbiterFee;
}

Within the Escrow#initiateDispute(), the i_arbiter would be validated whether or not the i_arbiter is address(0). Once the validation would be passed, the state of the State.Disputed would be stored into the s_state like this:
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L103-L104

/// @inheritdoc IEscrow
function initiateDispute() external onlyBuyerOrSeller inState(State.Created) {
if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter(); /// @audit
s_state = State.Disputed;
emit Disputed(msg.sender);
}

Within the Escrow#constructor() above, an arbiter address, which is assigned into the arbiter parameter, is supposed to be checked whether or not the arbiter address would not be address(0).

However, within the Escrow#constructor() above, there is no input validation to check whether or not the arbiter address would not be address(0).

If a seller is missing to assign an arbiter address into the arbiter parameter when the Escrow#constructor() would be called to create a new Escrow via the EscrowFactory#newEscrow(), both the seller and buyer can not initiate a dispute when they call the Escrow#initiateDispute() due to being reverted by the validation (at the Escrow.sol#L103) in the Escrow#initiateDispute() below:

if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter();

Impact

Both the seller and buyer can not initiate a dispute when they call the Escrow#initiateDispute().

Tools Used

  • Foundry

Recommendations

Within the Escrow#constructor(), consider adding an input validation to check whether or not the arbiter address would not be address(0) like this:

constructor(
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) {
+ require(arbiter != address(0), "The arbiter must not be address(0)");
...

Also, consider adding a setter function to set an arbiter address to the i_arbiter so that a seller can set an an arbiter address to the i_arbiter even if the seller was missing to assign an arbiter address into the arbiter parameter when the Escrow#constructor() would be called via the EscrowFactory#newEscrow() like this:

+ function setNewArbiter(address newArbiter) external onlySeller inState(State.Created) {
+ i_arbiter = newArbiter;
+ }

Support

FAQs

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

Give us feedback!