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

Due to lack of code to set the state of the `State.Created` to the `s_state` in the Escrow#`constructor()`, the key functions would always be reverted

0xmuxyz

Summary

Due to lack of code to set the state of the State.Created to the s_state in the Escrow#constructor(), the following three functions would always be reverted.

  • Escrow#confirmReceipt()

  • Escrow#initiateDispute()

  • Escrow#resolveDispute()

Vulnerability Details

EscrowFactory#newEscrow(), new Escrow contract would be created like this:
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L40-L47

/// @inheritdoc IEscrowFactory
/// @dev msg.sender must approve the token contract to spend the price amount before calling this function.
/// @dev There is a risk that if a malicious token is used, the dispute process could be manipulated.
/// Therefore, careful consideration should be taken when chosing the token.
function newEscrow(
uint256 price,
IERC20 tokenContract,
address seller,
address arbiter,
uint256 arbiterFee,
bytes32 salt
) external returns (IEscrow) {
...
Escrow escrow = new Escrow{salt: salt}( /// @audit
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
...
}

Within the Escrow#constructor(), each input value would be validated and stored into the each state variables like this:
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L32-L51

/// @dev Sets the Escrow transaction values for `price`, `tokenContract`, `buyer`, `seller`, `arbiter`, `arbiterFee`. All of
/// these values are immutable: they can only be set once during construction and reflect essential deal terms.
/// @dev Funds should be sent to this address prior to its deployment, via create2. The constructor checks that the tokens have
/// been sent to this address.
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;
}

Within the Escrow contract, the inState() modifier would be defined to check whether or not the current state is a proper state to call a target function like this:
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L82-L87

/// @dev Throws if contract called in State other than one associated for function.
modifier inState(State expectedState) { /// @audit
if (s_state != expectedState) {
revert Escrow__InWrongState(s_state, expectedState);
}
_;
}

Within the following three functions, whether or not the current state is the State.Created would be checked via the inState() modifier like this:

  • Escrow#confirmReceipt()
    https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L94

function confirmReceipt() external onlyBuyer inState(State.Created) { /// @audit

  • Escrow#initiateDispute()
    https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L102

function initiateDispute() external onlyBuyerOrSeller inState(State.Created) {　 /// @audit

  • Escrow#resolveDispute()
    https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L109

function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {　 /// @audit

When the Escrow#constructor() would be called (which means that a new Escrow is created), the state of the State.Created is supposed to be set to the s_state.
However, within the Escrow#constructor(), there is no code to set the State.Created to the s_state. This means that the state of the State.Created will never be set to the s_state.

Due to that, these three functions above would always be reverted.

Impact

Due to lack of code to set the state of the State.Created to the s_state in the Escrow#constructor(), the following three functions would always be reverted.

  • Escrow#confirmReceipt()

  • Escrow#initiateDispute()

  • Escrow#resolveDispute()

Tools Used

  • Manual review

Recommendations

Within the Escrow#constructor(), consider adding the code in order to set the State.Created to the s_state like this:

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;
+ s_state = State.Created;
}

Support

FAQs

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

Give us feedback!