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

Due to lack of the input validation to check whether or not `buyer != arbiter`, which allow a malicious buyer to be able to control the resolution of the dispute

0xmuxyz

Summary

Due to lack of the input validation to check whether or not buyer != arbiter in the Escrow#constructor(), the buyer can control when the dispute would be resolved. And therefore, this buyer can keep ignoring a dispute, which is initiated by a seller. (The worst scenario is that a dispute, which is initiated by a seller, will never be resolved forever due to that a malicious buyer can control when the dispute would be resolved)

Vulnerability Details

Within the EscrowFactory#newEscrow(), arbitrary addresses would be assign into the seller parameter and the arbiter parameter respectively. Then, they (seller and arbiter) and the caller address (msg.sender) would be assigned into the new Escrow() as the argument like this:
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L23-L24
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L43-L45

/// @inheritdoc IEscrowFactory
/// @dev msg.sender must approve the token contract to spend the price amount before calling this function.
/// @dev There is a risk that if a malicious token is used, the dispute process could be manipulated.
/// Therefore, careful consideration should be taken when chosing the token.
function newEscrow(
uint256 price,
IERC20 tokenContract,
address seller, /// @audit
address arbiter, /// @audit
uint256 arbiterFee,
bytes32 salt
) external returns (IEscrow) {
...
Escrow escrow = new Escrow{salt: salt}(
price,
tokenContract,
msg.sender, /// @audit
seller, /// @audit
arbiter, /// @audit
arbiterFee
);
...

Within the Escrow#constructor(), each address assigned via the EscrowFactory#newEscrow() above would be assigned into the each parameter (buyer, seller, arbiter).
Then, the buyer and seller would be validated whether or not each address would not be the address(0).
Once these validations above would be passed, each addresses (buyer, seller, arbiter) would be assigned into the each state variable (i_buyer, i_seller, i_arbiter) respectively like this:
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L35-L37
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L41-L42
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L47-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, /// @audit
address seller, /// @audit
address arbiter, /// @audit
uint256 arbiterFee
) {
if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
if (buyer == address(0)) revert Escrow__BuyerZeroAddress(); /// @audit
if (seller == address(0)) revert Escrow__SellerZeroAddress(); /// @audit
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; /// @audit
i_seller = seller; /// @audit
i_arbiter = arbiter; /// @audit
i_arbiterFee = arbiterFee;
}

However, within the Escrow#constructor(), there is no input validation to check whether or not buyer != arbiter.
If a buyer assign the buyer's address him/herself into the both buyer parameter and arbiter parameter when the buyer call the Escrow#constructor() via the EscrowFactory#newEscrow(), the buyer (i_buyer) can also become the arbiter (i_arbiter)

This lead to a bad situation that if a seller initiate a dispute, that dispute may never be resolved unless the arbiter, who is also the buyer, would resolve it by calling the Escrow#resolveDispute().
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L109-L129

Because in this case, the arbiter is also the buyer, meaning that the buyer can control when the dispute would be resolved. And therefore, this buyer can keep ignoring a dispute, which is initiated by a seller.

Impact

The buyer can control when the dispute would be resolved. And therefore, this buyer can keep ignoring a dispute, which is initiated by a seller. (The worst scenario is that a dispute, which is initiated by a seller, will never be resolved forever due to that a malicious buyer can control when the dispute would be resolved)

Tools Used

  • Foundry

Recommendations

Within the Escrow#constructor(), consider adding the input validations in order to check whether or not buyer != arbiter like this:
(NOTE：In this case, a seller should not be becoming a arbiter as well. So, the input validation to check whether or not seller != arbiter would also be added)

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

Support

FAQs

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