Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose revealDepositWithExtraData in the Bridge contract #760

Merged
merged 12 commits into from
Jan 9, 2024
Merged

Expose revealDepositWithExtraData in the Bridge contract #760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
727e669
Expose `revealDepositWithExtraData` from the `Bridge` contract
lukasz-zimnoch Dec 15, 2023
23aa5fc
Extract `_emitDepositRevealedEvent` to overcome the `stack too deep` …
lukasz-zimnoch Dec 18, 2023
182988d
Store `extraData` as part of `DepositRequest` struct
lukasz-zimnoch Dec 18, 2023
481d020
Add documentation to the recent changes around `Bridge` and extra data
lukasz-zimnoch Dec 18, 2023
dfe6d88
Adjust `WalletProposalValidator` to properly support deposits with ex…
lukasz-zimnoch Dec 19, 2023
d12b0e1
Adjust failing unit tests
lukasz-zimnoch Dec 19, 2023
4ad2b60
Unit tests covering the `revealDepositWithExtraData` function
lukasz-zimnoch Dec 20, 2023
2a7d54b
Disambiguate the nomenclature within `WalletProposalValidator` contract
lukasz-zimnoch Dec 21, 2023
78dc18c
Unit tests covering changes in the `WalletProposalValidator` contract
lukasz-zimnoch Dec 21, 2023
103411a
Merge branch 'main' into deposit-extra-data
lukasz-zimnoch Dec 21, 2023
a78cc16
Merge branch 'main' into deposit-extra-data
lukasz-zimnoch Dec 22, 2023
165c474
Merge branch 'main' into deposit-extra-data
lukasz-zimnoch Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions solidity/contracts/bridge/Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,35 @@ contract Bridge is
self.revealDeposit(fundingTx, reveal);
}

/// @notice Sibling of the `revealDeposit` function. This function allows
/// to reveal a P2(W)SH Bitcoin deposit with 32-byte extra data
/// embedded in the deposit script. The extra data allows to
/// attach additional context to the deposit. For example,
/// it allows a third-party smart contract to reveal the
/// deposit on behalf of the original depositor and provide
/// additional services once the deposit is handled. In this
/// case, the address of the original depositor can be encoded
/// as extra data.
/// @param fundingTx Bitcoin funding transaction data, see `BitcoinTx.Info`.
/// @param reveal Deposit reveal data, see `RevealInfo struct.
/// @param extraData 32-byte deposit extra data.
/// @dev Requirements:
/// - All requirements from `revealDeposit` function must be met,
/// - `extraData` must not be bytes32(0),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? My first thought was that it is to protect users but at this point, the Bitcoin transaction with 0x0 extra data is probably already in the Bitcoin mempool. I am not saying this condition is wrong, just curious about the reasons.

Copy link
Member Author

@lukasz-zimnoch lukasz-zimnoch Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning was: I don't see a reason to use revealDepositWithExtraData with empty extra data. Allowing to do so is a corner case. Such corner cases are often vectors of attack. This brought me to the security vs flexibility dilemma. In this case, I choose security. If there is a strong need to allow this case, we can always lift it in the future. Moreover, this requirement nicely separates revealDepositWithExtraData from revealDeposit.

Also, if someone does such a Bitcoin transaction with 0x0 extra data on purpose, there is always the refund option.

/// - `extraData` must be the actual extra data used in the P2(W)SH
/// BTC deposit transaction.
///
/// If any of these requirements is not met, the wallet _must_ refuse
/// to sweep the deposit and the depositor has to wait until the
/// deposit script unlocks to receive their BTC back.
function revealDepositWithExtraData(
BitcoinTx.Info calldata fundingTx,
Deposit.DepositRevealInfo calldata reveal,
bytes32 extraData
) external {
self.revealDepositWithExtraData(fundingTx, reveal, extraData);
}

/// @notice Used by the wallet to prove the BTC deposit sweep transaction
/// and to update Bank balances accordingly. Sweep is only accepted
/// if it satisfies SPV proof.
Expand Down
183 changes: 156 additions & 27 deletions solidity/contracts/bridge/Deposit.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,25 @@ import "./Wallets.sol";
/// Since each depositor has their own Ethereum address and their own
/// blinding factor, each depositor’s script is unique, and the hash
/// of each depositor’s script is unique.
///
/// This library also supports another variant of the deposit script
/// allowing to embed 32-byte extra data. The extra data allows to attach
/// additional context to the deposit. The script with 32-byte extra data
/// looks like this:
///
/// ```
/// <depositorAddress> DROP
/// <extraData> DROP
/// <blindingFactor> DROP
/// DUP HASH160 <walletPubKeyHash> EQUAL
/// IF
/// CHECKSIG
/// ELSE
/// DUP HASH160 <refundPubkeyHash> EQUALVERIFY
/// <refundLocktime> CHECKLOCKTIMEVERIFY DROP
/// CHECKSIG
/// ENDIF
/// ```
library Deposit {
using BTCUtils for bytes;
using BytesLib for bytes;
Expand Down Expand Up @@ -96,6 +115,8 @@ library Deposit {
// the time when the sweep proof was delivered to the Ethereum chain.
// XXX: Unsigned 32-bit int unix seconds, will break February 7th 2106.
uint32 sweptAt;
// The 32-byte deposit extra data. Optional, can be bytes32(0).
bytes32 extraData;
// This struct doesn't contain `__gap` property as the structure is stored
// in a mapping, mappings store values in different slots and they are
// not contiguous with other values.
Expand Down Expand Up @@ -152,6 +173,28 @@ library Deposit {
BitcoinTx.Info calldata fundingTx,
DepositRevealInfo calldata reveal
) external {
_revealDeposit(self, fundingTx, reveal, bytes32(0));
}

/// @notice Internal function encapsulating the core logic of the deposit
/// reveal process. Handles both regular deposits without extra data
/// as well as deposits with 32-byte extra data embedded in the
/// deposit script. The behavior is controlled by the `extraData`
/// parameter. If `extraData` is bytes32(0), the function triggers
/// the flow for regular deposits. If `extraData` is not bytes32(0),
/// the function triggers the flow for deposits with 32-byte
/// extra data.
/// @param fundingTx Bitcoin funding transaction data, see `BitcoinTx.Info`.
/// @param reveal Deposit reveal data, see `RevealInfo struct.
/// @param extraData 32-byte deposit extra data. Can be bytes32(0).
/// @dev Requirements are described in the docstrings of `revealDeposit` and
/// `revealDepositWithExtraData` external functions.
function _revealDeposit(
BridgeState.Storage storage self,
BitcoinTx.Info calldata fundingTx,
DepositRevealInfo calldata reveal,
bytes32 extraData
) internal {
require(
self.registeredWallets[reveal.walletPubKeyHash].state ==
Wallets.WalletState.Live,
Expand All @@ -167,33 +210,70 @@ library Deposit {
validateDepositRefundLocktime(self, reveal.refundLocktime);
}

bytes memory expectedScript = abi.encodePacked(
hex"14", // Byte length of depositor Ethereum address.
msg.sender,
hex"75", // OP_DROP
hex"08", // Byte length of blinding factor value.
reveal.blindingFactor,
hex"75", // OP_DROP
hex"76", // OP_DUP
hex"a9", // OP_HASH160
hex"14", // Byte length of a compressed Bitcoin public key hash.
reveal.walletPubKeyHash,
hex"87", // OP_EQUAL
hex"63", // OP_IF
hex"ac", // OP_CHECKSIG
hex"67", // OP_ELSE
hex"76", // OP_DUP
hex"a9", // OP_HASH160
hex"14", // Byte length of a compressed Bitcoin public key hash.
reveal.refundPubKeyHash,
hex"88", // OP_EQUALVERIFY
hex"04", // Byte length of refund locktime value.
reveal.refundLocktime,
hex"b1", // OP_CHECKLOCKTIMEVERIFY
hex"75", // OP_DROP
hex"ac", // OP_CHECKSIG
hex"68" // OP_ENDIF
);
bytes memory expectedScript;

if (extraData == bytes32(0)) {
// Regular deposit without 32-byte extra data.
expectedScript = abi.encodePacked(
hex"14", // Byte length of depositor Ethereum address.
msg.sender,
hex"75", // OP_DROP
hex"08", // Byte length of blinding factor value.
reveal.blindingFactor,
hex"75", // OP_DROP
hex"76", // OP_DUP
hex"a9", // OP_HASH160
hex"14", // Byte length of a compressed Bitcoin public key hash.
reveal.walletPubKeyHash,
hex"87", // OP_EQUAL
hex"63", // OP_IF
hex"ac", // OP_CHECKSIG
hex"67", // OP_ELSE
hex"76", // OP_DUP
hex"a9", // OP_HASH160
hex"14", // Byte length of a compressed Bitcoin public key hash.
reveal.refundPubKeyHash,
hex"88", // OP_EQUALVERIFY
hex"04", // Byte length of refund locktime value.
reveal.refundLocktime,
hex"b1", // OP_CHECKLOCKTIMEVERIFY
hex"75", // OP_DROP
hex"ac", // OP_CHECKSIG
hex"68" // OP_ENDIF
);
} else {
// Deposit with 32-byte extra data.
expectedScript = abi.encodePacked(
hex"14", // Byte length of depositor Ethereum address.
msg.sender,
hex"75", // OP_DROP
hex"20", // Byte length of extra data.
extraData,
hex"75", // OP_DROP
hex"08", // Byte length of blinding factor value.
reveal.blindingFactor,
hex"75", // OP_DROP
hex"76", // OP_DUP
hex"a9", // OP_HASH160
hex"14", // Byte length of a compressed Bitcoin public key hash.
reveal.walletPubKeyHash,
hex"87", // OP_EQUAL
hex"63", // OP_IF
hex"ac", // OP_CHECKSIG
hex"67", // OP_ELSE
hex"76", // OP_DUP
hex"a9", // OP_HASH160
hex"14", // Byte length of a compressed Bitcoin public key hash.
reveal.refundPubKeyHash,
hex"88", // OP_EQUALVERIFY
hex"04", // Byte length of refund locktime value.
reveal.refundLocktime,
hex"b1", // OP_CHECKLOCKTIMEVERIFY
hex"75", // OP_DROP
hex"ac", // OP_CHECKSIG
hex"68" // OP_ENDIF
);
}

bytes memory fundingOutput = fundingTx
.outputVector
Expand Down Expand Up @@ -258,6 +338,21 @@ library Deposit {
deposit.treasuryFee = self.depositTreasuryFeeDivisor > 0
? fundingOutputAmount / self.depositTreasuryFeeDivisor
: 0;
deposit.extraData = extraData;

_emitDepositRevealedEvent(fundingTxHash, fundingOutputAmount, reveal);
}

/// @notice Emits the `DepositRevealed` event.
/// @param fundingTxHash The funding transaction hash.
/// @param fundingOutputAmount The funding output amount in satoshi.
/// @param reveal Deposit reveal data, see `RevealInfo struct.
/// @dev This function is extracted to overcome the stack too deep error.
function _emitDepositRevealedEvent(
bytes32 fundingTxHash,
uint64 fundingOutputAmount,
DepositRevealInfo calldata reveal
) internal {
// slither-disable-next-line reentrancy-events
emit DepositRevealed(
fundingTxHash,
Expand All @@ -272,6 +367,40 @@ library Deposit {
);
}

/// @notice Sibling of the `revealDeposit` function. This function allows
/// to reveal a P2(W)SH Bitcoin deposit with 32-byte extra data
/// embedded in the deposit script. The extra data allows to
/// attach additional context to the deposit. For example,
/// it allows a third-party smart contract to reveal the
/// deposit on behalf of the original depositor and provide
/// additional services once the deposit is handled. In this
/// case, the address of the original depositor can be encoded
/// as extra data.
/// @param fundingTx Bitcoin funding transaction data, see `BitcoinTx.Info`.
/// @param reveal Deposit reveal data, see `RevealInfo struct.
/// @param extraData 32-byte deposit extra data.
/// @dev Requirements:
/// - All requirements from `revealDeposit` function must be met,
/// - `extraData` must not be bytes32(0),
/// - `extraData` must be the actual extra data used in the P2(W)SH
/// BTC deposit transaction.
///
/// If any of these requirements is not met, the wallet _must_ refuse
/// to sweep the deposit and the depositor has to wait until the
/// deposit script unlocks to receive their BTC back.
function revealDepositWithExtraData(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this is not a good opportunity to allow anyone to reveal the deposit. In revealDeposit, the requirements would be just as they are now because the vault address is not a part of the P2(W)SH script. In revealDepositWithExtraData, we use another version of the script so we could expect the vault address to be inside the script.

We could also lift the requirements of not having extra data empty (see my previous comment) and potentially allow to use revealDepositWithExtraData for tBTC vault. In this model, revealDeposit is the cheaper version that has come constraints allowing to make it cheaper. For anyone who needs more flexibility at the expense of paying more for gas, revealDepositWithExtraData can be used.

I think we had this discussion before but I do not remember why we did not decide on this option. Having a separate depositor contract may not be needed in all cases and I am not even sure if it is needed for Solana/Base, cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not only about the vault. Most importantly, the depositor embedded in the script is the receiver of minted TBTC (or Bank's balance). If something has to happen with minted TBTC automatically, the depositor must be aware of the deposit. In most cases, the depositor will also need to record some information upon deposit reveal (e.g. current fee values) to properly do the automation on minted TBTC (e.g. compute the net amount of the deposit). Requiring that only the depositor can reveal guarantees that the deposit always passes through the depositor before reaching the Bridge.

Let's use Base as an example: While making a deposit, we are setting the BaseDepositor contract as depositor and the Base EOA as the original depositor in extra data. If we allow anyone to reveal, a third party can just reveal that deposit to the Bridge and bypass the BaseDepositor contract. The BaseDepositor will receive minted TBTC but will not be aware of the deposit from the very beginning. Although it's probably possible to deliver that information later, this can be very tricky in some cases and complicates the relayer bot. Moreover, BaseDepositor hasn't had a chance to capture the Bridge state upon deposit reveal so doing the right thing with minted TBTC can be hard. This example also shows why we need the depositor contract for Base: it's not only about revealing the deposit but also about moving back minted TBTC back to Base.

Last but not least, I'd say that not having the vault in the script is more flexible and safe. What if the vault is set as untrusted due to a critical vulnerability? If the vault is embedded in the script, the deposit can be revealed there at any time by anyone, far before the refund. The vault cannot be changed in that case. If we don't embed, the deposit can always be revealed with vault address(0).

That said, I see more problems than advantages with letting anyone reveal the deposit. The revealDepositWithExtraData covers the flexibility gap that was caused by this requirement in the original revealDeposit and still provides strong security guarantees.

BridgeState.Storage storage self,
BitcoinTx.Info calldata fundingTx,
DepositRevealInfo calldata reveal,
bytes32 extraData
) external {
// Strong requirement in order to differentiate from the regular
// reveal flow and reduce potential attack surface.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular attack surface? If the vault doesn't use extra data as in the example of tBTC, this is just about mapping the revealed deposit to the Bitcoin script to unlock UTXO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see: #760 (comment)

require(extraData != bytes32(0), "Extra data must not be empty");

_revealDeposit(self, fundingTx, reveal, extraData);
}

/// @notice Validates the deposit refund locktime. The validation passes
/// successfully only if the deposit reveal is done respectively
/// earlier than the moment when the deposit refund locktime is
Expand Down
Loading
Loading