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

Deployed contracts by the `EscrowFactory` are not saved

0xhals

Summary

Deployed contracts by the EscrowFactory are not saved

Vulnerability Details

As there's no mapping to track the deployed escrow contracts via the factory in the EscrowFactory; any buyer can deploy an escrow contract directly (not through the factory) .

Impact

Informational: no impact on the security of the contract

Proof of Concept

Escrow contract/constructor

Line 32-51:
constructor(
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
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;
i_arbiterFee = arbiterFee;
}

Tools Used

Manual Testing.

Recommendations

  1. In the Escrow constructor: check that the deployer msg.sender is the factory contract:

+ Address private escrowFactory=0xab..........; //hardcoded address of the escrow factory
constructor(
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
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();
+ if (msg.sender != escrowFactory) revert();
i_price = price;
i_tokenContract = tokenContract;
i_buyer = buyer;
i_seller = seller;
i_arbiter = arbiter;
i_arbiterFee = arbiterFee;
}

  1. In the EscrowFactory contract: add a mapping to track all deployed escrows via this contract, so that these contracts only to be rendered in the frontend:

+ address[] public deployedEscrows;
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();
}
+ deployedEscrows.push(address(escrow));
emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);
return escrow;
}

Support

FAQs

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