Cyfrin / 2023-07-escrow

16 stars 12 forks source link

Compromised/bribed/keys lost arbiter may lock funds in the contract forever #592

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

Compromised/bribed/keys lost arbiter may lock funds in the contract forever

Severity

Medium Risk

Relevant GitHub Links

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

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

Summary

The state machine of the Escrow does not allow to change the state from Disputed otherwise than by calling resolveDispute(), which can be only called by arbiter. That means that compromised, bribed, or one with keys lost cannot resolve the dispute, locking the funds in the smart contract forever.

Vulnerability Details

First step, anyone can initiate a dispute: https://github.com/Cyfrin/2023-07-escrow/blob/65a60eb0773803fa0be4ba72defaec7d8567bccc/src/Escrow.sol#L102-L106

    function initiateDispute() external onlyBuyerOrSeller inState(State.Created) {
        if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter();
        s_state = State.Disputed;
        emit Disputed(msg.sender);
    }

Then, in order to resolve a dispute, it has to be called by arebiter and the state has to be Disputed: https://github.com/Cyfrin/2023-07-escrow/blob/65a60eb0773803fa0be4ba72defaec7d8567bccc/src/Escrow.sol#L109

    function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed)

In case that arbiter never calls it, the funds are locked forever.

Impact

Funds lost in case that arbiter does not resolve dispute.

Tools Used

Manual analysis.

Recommendations

Consider making dispute auto-resolvable after specific amount of time. For example after a month since starting a dispute both buyer and seller are able to get half of the escrowed funds and arbiter does not receive anything, because they did not do what they should.

PatrickAlphaC commented 1 year ago

arbitor is a trusted role