ERC20 Contract Validation Missing: Ensuring Contract Compliance in the Escrow Contract"

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-escrow/blob/65a60eb0773803fa0be4ba72defaec7d8567bccc/src/Escrow.sol#L32

Summary

The contract does not validate if the provided token contract address actually adheres to the ERC20 standard. This could lead to unexpected behavior or potential loss of funds if a non-ERC20 compliant contract or malicious contract address is provided.

Vulnerability Details

In the constructor of the Escrow contract, the address of the token contract is accepted as a parameter. While there is a check to ensure that the address provided is not a zero address, there is no check to ensure that the address provided is a valid ERC20 contract.

constructor(
    uint256 price,
    IERC20 tokenContract,
    address buyer,
    address seller,
    address arbiter,
    uint256 arbiterFee
) {
    if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
    ...
    i_tokenContract = tokenContract;
    ...
}

A malicious or erroneous token contract could potentially cause unexpected behavior within the Escrow contract. For instance, a malicious contract could be designed to always return true when the balanceOf or transfer function is called, regardless of the actual balance or transfer outcome.

Impact

The impact of this vulnerability could range from minor to severe, depending on the provided token contract. In the best-case scenario, the Escrow contract simply wouldn't work as expected if a non-compliant ERC20 contract is provided. In the worst-case scenario, a malicious contract could lead to loss of funds or other unexpected behavior.

Tools Used

This vulnerability was found using manual code review methods.

Recommendations

I recommend adding a check to ensure that the provided token contract adheres to the ERC20 standard. This can be done by calling a function on the contract and checking the result.

Here is an example of how this could be done:

require(
    tokenContract.totalSupply() > 0,
    "Token contract must implement totalSupply()"
);

This will throw an error if the totalSupply function is not implemented by the contract, which is a required function in the ERC20 standard. Note that this is a basic check and might not catch all non-compliant contracts. For a more robust solution, consider using an ERC20 verifier library or service.

Cyfrin / 2023-07-escrow