decred / atomicswap

On-chain atomic swaps for Decred and other cryptocurrencies.
https://blog.decred.org/2017/09/20/On-Chain-Atomic-Swaps/
ISC License
508 stars 231 forks source link

Atomic swap smart contracts must check secret size. #58

Closed markblundeberg closed 6 years ago

markblundeberg commented 6 years ago

Your cross-chain atomic swaps appear to be using the canonical form of smart contracts, which can make them vulnerable to an attack that I describe here:

https://gist.github.com/markblundeberg/7a932c98179de2190049f5823907c016

If Alice is providing the secret, then the smart contract created by Bob must verify the size of the secret or else he is vulnerable to having his money stolen.

jrick commented 6 years ago

Thank you for bringing this to our attention. You are certainly correct that fraud could be performed between two currencies that have different maximum script push sizes, and that placing and verifying the actual length in the contract allows it to be audited.

We will make the appropriate changes to add the extra validation, and not accept any implementations for coins with a different maximum script push size until this is resolved. Just to reduce complexity, I think only working with a single known good secret size is best, instead of trying to support anything under the maximum.

If you haven't yet reported it, please bring this to the attention of the BIP0199 authors, as their example script is similarly vulnerable and the BIP will be widely referenced by implementors.

markblundeberg commented 6 years ago

Thanks for the fast response!

I agree with what you say --- the simplest fix is that all contracts should be checked for standard form that includes a hardcoded secret size check of 32 bytes. No variation allowed.

I've sent an email to the authors you suggested, cheers!