Escrow contract vulnerability

[Austin-Williams]

Issue

Attacker can DoS payouts from escrow for trades in which they are the buyer.

Severity

Medium. The attacker must be running well-connected ETH nodes to perform the attack. The attacker can use the ability to DoS payouts to extort funds from sellers. The attack is active and requires ongoing resources to persist.

The Attack

An attacker can listen for a call to execute where the scriptHash passed is one corresponding to a trade in which they are the buyer. If the attacker wants to block the payout they can do so by calling addFundsToTransaction with 1 wei and paying a higher gas cost than the was used when the honest party called execute.

(Similarly, if the trade was an ERC20-based trade, they would call addTokensToTransaction with the smallest unit of value possible).

If the attacker's transaction is mined first (and it should be if they are well connected and paying a higher gas cost) the result is that the value in the Transaction struct will be one wei higher than the sum of the amounts[] passed to execute by the honest party. This means that the call to execute by the honest party will revert (at line 375 of Escrow_v1_0.sol) because the total amount of the payouts (that is, the sum of amounts[]) does not exactly match the value of the corresponding Transaction struct.

The attacker can use this ability to DoS payouts to extort funds from the seller.

Tl;Dr: The attacker can basically "front-run" escrow payout txns with a call to addFundsToTransaction, resulting in a failure of the escrow payout txn.

Proposed Fix

In the next version of the contract I proposed the following changes:

1. On line 375-378 of Escrow_v1_0.sol you'll see:

require(
            _transferFunds(scriptHash, destinations, amounts) == transactions[scriptHash].value,
            "Total value to be released must be equal to the transaction escrow value"
        );

I propose that gets changed to:

require(
            _transferFunds(scriptHash, destinations, amounts) <= transactions[scriptHash].value,
            "Total value to be released must be no more than the transaction escrow value"
        );

Basically, we allow the users the ability to send less than the total value of the trade out of escrow. This completely eliminates the DoS vulnerability, though it introduces the possibility that a front-end error could result in the permanent loss of user funds. For example, if the front-end initiates a payout of less than 100% of the escrowed funds, all funds remaining in escrow will be stuck forever. So if we make this change, we should also make the following two changes.

2. After sending money out of Escrow, track the amount sent (for example, update transactions[scriptHash].value by reducing it by the amount paid out).

3. Give users the ability to spend any remaining funds held in escrow when state is RELEASED. This could be done, for example, by removing the inFundedState modifier from the execute function. While actually performing this action should be rare, it is important that it exists, or else front-end implementation errors could result in permanent loss of user funds (which, IIR, was the motivation behind using == instead of <= to begin with).

Conclusion

Somehow this got by both our internal audit as well as Zeppelin's audit of the escrow contract. It's not a high severity issue but I think it should be addressed in the next version of the escrow contract. Props to @tyler-smith for noticing this issue.

[sameepsi]

Another thing which we can implement is to restrict the buyer to call addTokensToTransaction/addFundsToTransaction only once for each transaction. Because ideally, buyer should not be calling that method more than once.

[Austin-Williams]

Yeah, that’s another path we could take. That would mean that ETH/ERC20 behavior would be different from the behavior we allow for other coins (BTC/ZEC/etc), because we allow buyers who use other coins to top up as often as they want. We also allow partial payouts from escrow for other coins (even though we don’t support that at the UI layer).

So the downside to this approach is that it makes ETH/ERC20 behavior differ from other coins.

The upside is that it’s slightly easier to implement.

We should bring it up with the backend devs and see what they think.

[sameepsi]

@Austin-Williams I like to proposal. But I would like to point out one thing which is instead of reducing transactions[scriptHash].value by the amount paid out we should keep track of released amount using different storage variable transactions[scriptHash].released.

By this architecture, the user can always get the total value of the transaction and the released amount.

[sameepsi]

I will fix this one and will add corresponding test cases for the same.

[Austin-Williams]

This has been resolve by PR #21. The strict equality requirement has been removed. Users can now send funds out of escrow more than once.