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

Misbehavior in `newEscrow` function due to Non-Standard ERC20 token contract

0xNiloy

Summary

The newEscrow function in the EscrowFactory contract assumes that the transferFrom function of the provided ERC20 token contract will behave as expected. However, if a malicious or non-standard ERC20 token is used, it could potentially manipulate the state of the escrow contract, leading to unexpected behavior.

Vulnerability Details

The newEscrow function is designed to create a new instance of the Escrow contract. It accepts several parameters, including a tokenContract parameter that represents the ERC20 token contract to be used for the transaction. The function calls the transferFrom method of this contract to transfer the price amount from the buyer to the escrow contract. However, there is no explicit check in the function to ensure that the transferFrom function of the provided ERC20 token contract behaves as expected. If a non-standard or malicious ERC20 token contract is used, it could potentially manipulate the state of the escrow contract.

Code Snippet

function newEscrow(
uint256 price,
IERC20 tokenContract,
address seller,
address arbiter,
uint256 arbiterFee,
bytes32 salt
) external returns (IEscrow) {
address computedAddress = computeEscrowAddress(
type(Escrow).creationCode,
address(this),
uint256(salt),
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
Escrow escrow = new Escrow{salt: salt}(
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
if (address(escrow) != computedAddress) {
revert EscrowFactory__AddressesDiffer();
}
emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);
return escrow;
}

Impact

If a malicious or non-standard ERC20 token contract is used, it could potentially manipulate the state of the escrow contract. This could lead to unexpected behavior, such as the transfer of funds not occurring as expected, or the balance of the escrow contract being manipulated. This could potentially result in a loss of funds for the buyer or the seller, and could undermine trust in the platform.

Tools Used

Manual code review

Recommendations

To mitigate this potential issue, it is recommended to add additional checks in the newEscrow function to ensure that the transferFrom function of the provided ERC20 token contract behaves as expected. This could be implemented by adding a require statement after the transferFrom call, like so:

tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
require(tokenContract.balanceOf(computedAddress) == price, "Transfer of tokens failed");

This would ensure that the correct amount of tokens has been transferred to the escrow contract, thereby preventing the potential issues described above.

Support

FAQs

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