Escrow contract stuck tokens

[thegostep]

The escrow contract can lead to stuck tokens if the operator deposits the stake when there is no staker set. The tokens become stuck once the tokens are transfered to the agreement contract since the zero address is set as the staker.

[fulldecent]

I am trying to understand this. Regarding: https://github.com/erasureprotocol/erasure-protocol/blob/c716c17a080ac7817312ab57ec4b1c60aa867f19/contracts/escrows/CountdownGriefingEscrow.sol

The scenario here is:

1. Operator creates the CountdownGriefingEscrow contract and initializes with:
   • seller = address(0)
   • operator ≠ address(0)
2. Operator deposits stake using depositStake
   • Documentation states "if seller not already set, make msg.sender the seller"
   • seller is not changed
   • Tokens are attributed to seller which is still address(0)
3. Nobody can get tokens that are attributed to address(0)

Design issues:

• If a contract is initialized where the seller is open-ended then the contracts fails if the operator acts as a seller.
• There is no way for an operator to set the seller for the contract if it was not set at initialization.
• The contract works properly if the sender on an open-ended contract deposits stake by itself.

Design questions:

• Is it desired that an operator will be able to set the sender if the contract was initialized without a sender?

Is this a vulnerability?:

• We need to see the contract implementation for the intended operator contract to decide.
• If the operator contract wants to set the sender for a contract that was initialized without a sender then then this is a vulnerability.

Remediation:

1. At a minimum, for safety, prevent operator from acting as seller in the current implementation.
   1. This can be done in below here
   2. OR review the functionality of the operator implementation to make sure it is not used in this way.
2. If it is desired that operator can set the seller at a time after contract initialization, then this must be added as a new public function.

[thegostep]

@fulldecent went for remediation num 4.

The rationale is that operator serves as a blanket bypass of all access control to the public functions of the contract. This allows for composability since I can delegate the permissions to a separate contract that only implements the desired functionality.

Hopefully the documentation makes clear that the operator is always a trusted party if used.

[thegostep]

The fix includes the ability to set an arbitrary seller / buyer instead of inferring it from msg.sender. Do you think this is a good pattern?

[fulldecent]

Looks great. Very clear and safe. Good pattern.