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

Reuse of Same parameter including Salt will revert

shanb1605

Summary

The EscrowFactory:newEscrow() function intakes 6 parameters including Salt If the Buyer deploys a second escrow contract with the same parameters and salt has been reused the creation of the escrow contract will be reverted.

Vulnerability Details

  1. A defi protocol buys an audit from Cyfrin audit and creates an escrow factory with the following parameters. EscrowFactory:newEscrow(100,0x1,address(cyfrin),0x2,10,0x0001)

  2. Address 0x32.. will be computed and an escrow contract will be created on address 0x32..

  3. The Same defi protocol wishes to give a second audit with the same price to Cyfrin. The Buyer(Defi Protocol) calls newEscrow() on EscrowFactory as the same parameters on the first audit as the following params EscrowFactory:newEscrow(100,0x1,address(cyfrin),0x2,10,0x0001)

  4. The Same Address 0x32.. will be computed but the creation of the escrow contract on 0x32.. will be reverted

Proof of Concept:

Run this Foundry Test code inside the test/unit/ folder of https://github.com/Cyfrin/2023-07-escrow/ Repo:

forge test -vv --mp test/unit/Escrow-Poc.t.sol

pragma solidity 0.8.18;
​
import {Test} from "forge-std/Test.sol";
import {DeployEscrowFactory} from "../../script/DeployEscrowFactory.s.sol";
import {EscrowTestBase} from "../EscrowTestBase.t.sol";
import {IEscrow, Escrow} from "../../src/Escrow.sol";
import {EscrowFactory} from "src/EscrowFactory.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import {ERC20MockFailedTransfer} from "../mocks/ERC20MockFailedTransfer.sol";
​
contract EscrowPoc is Test, EscrowTestBase {
EscrowFactory public escrowFactory;
IEscrow public escrow;
IEscrow public escrow2;
​
function setUp() public {
DeployEscrowFactory deployer = new DeployEscrowFactory();
escrowFactory = deployer.run();
}
function test_Poc() public {
vm.startPrank(BUYER);
emit log_string("Buyer Initializes the 1st audit");
ERC20Mock(address(i_tokenContract)).mint(BUYER, PRICE);
ERC20Mock(address(i_tokenContract)).approve(address(escrowFactory), PRICE);
escrow = escrowFactory.newEscrow(PRICE, i_tokenContract, SELLER, ARBITER, ARBITER_FEE, SALT1);
​
emit log_string("Buyer Initializes the 2nd audit with Same params From 1st audit");
ERC20Mock(address(i_tokenContract)).mint(BUYER, PRICE);
ERC20Mock(address(i_tokenContract)).approve(address(escrowFactory), PRICE);
​
emit log_string("Expected revert Because of Using same Salt by Buyer.......");
vm.expectRevert();
escrow2 = escrowFactory.newEscrow(PRICE, i_tokenContract, SELLER, ARBITER, ARBITER_FEE, SALT1);
vm.stopPrank();
}
​
}

The Foundry test code contains vm.expectRevert() where the test case passes means The user creates escrow with same params and salt will be reverted on the creation of the second deployment

Impact

Hence, the creation of a second deployment of escrow will be reverted, if the user uses the same Salt and params.

Tools Used

Manual Review, Foundry

Recommendations

I recommend this fix:

contract EscrowFactory is IEscrowFactory {
using SafeERC20 for IERC20;
​
//Array to Store User Salt Value
mapping(address => uint256) public SaltOf;
​
​
function newEscrow(
uint256 price,
IERC20 tokenContract,
address seller,
address arbiter,
uint256 arbiterFee
//Ignore User Input Salt Value
)
external
returns (
IEscrow
)
{
//Creating unique salt on Each Call
uint salt = SaltOf[msg.sender]++;
​
address computedAddress = computeEscrowAddress(
type(Escrow).creationCode,
address(this),
uint256(salt),
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
​
//uint256 Salt is converted to bytes32() on creation of contract
Escrow escrow = new Escrow{salt: bytes32(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;
}

  • There should be a public array for Storing the Salt value of the user.

  • User input of Salt is ignored Hence it also solves the known issue of FrontRunning

  • Salt Array should be increment on each call hence it's used on the computeEscrowAddress() function.

  • On Creation by new Escrow{salt: bytes32(salt)} the unit salt should be converted to bytes32(salt).

Support

FAQs

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