search

keep-network / tbtc

Trustlessly tokenized Bitcoin on Ethereum ;)
https://tbtc.network
MIT License
213 stars 40 forks source link

P2PKH and P2SH outputs are accepted for funding proofs #664

Closed samczsun closed 4 years ago

samczsun commented 4 years ago

Description

When providing a funding proof, three types of outputs are accepted: P2WPKH, P2PKH, and P2SH outputs. The system is designed to handle the first and fails for the second and third.

If a P2PKH output is provided, the signing group is able to generate a scriptSig which can spend the output. However, that will trigger the ECDSA fraud flow as the signature wasn't authorized.

If a P2SH output is provided, then the public key of the keep will be interpreted as a script. This most likely results in a locked output.

Exploit Scenario

Eve creates a deposit, funds it using a P2PKH or P2SH output, and requests redemption. The signing group is unable to provide a redemption proof, and eventually Eve seizes the group's bonds.

Suggested Remediation

Disallow P2PKH and P2SH outputs when providing funding proofs.

Shadowfiend commented 4 years ago

The solution here (via @prestwich) will be to only allow p2wpkh. We assume p2wpkh in the redemption flow anyway.

Checking for _output.keccak256Slice(8, 23) == keccak256(abi.encodePacked(hex"160014", signerPKH(_d))) and _output.length == 31 instead of the extractHash check in findAndParseFundingOutput should do the trick:

https://github.com/keep-network/tbtc/blob/2a906cc3f070983535763cf0e5185ee5fb65b28a/solidity/contracts/deposit/DepositUtils.sol#L170

Shadowfiend commented 4 years ago

This is the same category of issue as #658 and #636, in that it allows a deposit to enter a state where its redemption cannot be proven, thus enabling a BTC-for-ETH bond exchange.