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

[H-01] If the Creation Escrow fail all the fund is loss

pina

Summary

If the Escrow contract creation fails for any reason, then the tokens will already have been transferred to an address which might not be accessible, resulting in the loss of these tokens.

Vulnerability Details

If the creation of the escrow in the newEscrow() function of the EscrowFactory.sol contract fails for any reason, the transferred funds will be lost.

This happens because not creating a contract throws an exception. Interrupt the execution of the operation and therefore not reach the proposed reversal to reverse the operation.

Therefore, if the token transfer is successful and then escrow creation fails, the token transfer is not rolled back.

A useful resource that helped me confirm this vulnerability was the link I shared, in the last paragraph it says:

If the creation fails (due to out-of-stack, not enough balance or other problems), an exception is thrown.

Impact

  • High, the buyer can lose all your funds.

Tools Used

  • Manual code review

Recommendations

To solve this problem, you can reorder the operations in such a way that the token transfer is done only after the Escrow contract has been successfully created and to make it more secure after the address has been verified.

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
);
Escrow escrow = new Escrow{salt: salt}(
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
if (address(escrow) != computedAddress) {
revert EscrowFactory__AddressesDiffer();
}
tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
if (tokenContract.balanceOf(computedAddress) < price) {
revert Escrow__MustDeployWithTokenBalance();
}
emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);
return escrow;
}
​

And delete from the constructor of Escrow because this statement will now be evaluated from the EscrowFactory:

- Escrow.sol:44: if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
​

In this case, if the creation of the Escrow contract fails or if the calculated address does not match the actual address of the contract, the transfer of tokens will not take place and there will be no risk of losing tokens.

Support

FAQs

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