From 727e669b711fb0cb27a8212d6c062d1705a5a4b9 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Fri, 15 Dec 2023 17:17:26 +0100 Subject: [PATCH 1/9] Expose `revealDepositWithExtraData` from the `Bridge` contract Here we expose the `revealDepositWithExtraData` function that allows revealing deposits with embedded 32-byte extra data. --- solidity/contracts/bridge/Bridge.sol | 12 ++- solidity/contracts/bridge/Deposit.sol | 121 ++++++++++++++++++++------ 2 files changed, 103 insertions(+), 30 deletions(-) diff --git a/solidity/contracts/bridge/Bridge.sol b/solidity/contracts/bridge/Bridge.sol index f4506e83f..8692e1988 100644 --- a/solidity/contracts/bridge/Bridge.sol +++ b/solidity/contracts/bridge/Bridge.sol @@ -82,7 +82,8 @@ contract Bridge is bytes20 indexed walletPubKeyHash, bytes20 refundPubKeyHash, bytes4 refundLocktime, - address vault + address vault, + bytes32 extraData ); event DepositsSwept(bytes20 walletPubKeyHash, bytes32 sweepTxHash); @@ -376,6 +377,15 @@ contract Bridge is self.revealDeposit(fundingTx, reveal); } + // TODO: Documentation and tests. + 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. diff --git a/solidity/contracts/bridge/Deposit.sol b/solidity/contracts/bridge/Deposit.sol index 0fb51d785..e2e3639e7 100644 --- a/solidity/contracts/bridge/Deposit.sol +++ b/solidity/contracts/bridge/Deposit.sol @@ -110,7 +110,8 @@ library Deposit { bytes20 indexed walletPubKeyHash, bytes20 refundPubKeyHash, bytes4 refundLocktime, - address vault + address vault, + bytes32 extraData ); /// @notice Used by the depositor to reveal information about their P2(W)SH @@ -152,6 +153,16 @@ library Deposit { BitcoinTx.Info calldata fundingTx, DepositRevealInfo calldata reveal ) external { + revealDeposit(self, fundingTx, reveal, bytes32(0)); + } + + // TODO: Documentation and think about better name. + 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, @@ -167,33 +178,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 @@ -268,10 +316,25 @@ library Deposit { reveal.walletPubKeyHash, reveal.refundPubKeyHash, reveal.refundLocktime, - reveal.vault + reveal.vault, + extraData ); } + // TODO: Documentation and tests. + function revealDepositWithExtraData( + 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. + 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 From 23aa5fc952b511662400438eadbdbd9bef44bd83 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Mon, 18 Dec 2023 13:41:27 +0100 Subject: [PATCH 2/9] Extract `_emitDepositRevealedEvent` to overcome the `stack too deep` error The number of local variables used in the `_revealDeposit` functions exceeds the allowed limit and causes the `stack too deep` error during compilation. To mitigate it, we are extracting `_emitDepositRevealedEvent` whose responsibility is to emit the `DepositRevealed` event. --- solidity/contracts/bridge/Deposit.sol | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/solidity/contracts/bridge/Deposit.sol b/solidity/contracts/bridge/Deposit.sol index e2e3639e7..11370c5e5 100644 --- a/solidity/contracts/bridge/Deposit.sol +++ b/solidity/contracts/bridge/Deposit.sol @@ -153,11 +153,11 @@ library Deposit { BitcoinTx.Info calldata fundingTx, DepositRevealInfo calldata reveal ) external { - revealDeposit(self, fundingTx, reveal, bytes32(0)); + _revealDeposit(self, fundingTx, reveal, bytes32(0)); } // TODO: Documentation and think about better name. - function revealDeposit( + function _revealDeposit( BridgeState.Storage storage self, BitcoinTx.Info calldata fundingTx, DepositRevealInfo calldata reveal, @@ -306,6 +306,27 @@ library Deposit { deposit.treasuryFee = self.depositTreasuryFeeDivisor > 0 ? fundingOutputAmount / self.depositTreasuryFeeDivisor : 0; + + _emitDepositRevealedEvent( + fundingTxHash, + fundingOutputAmount, + reveal, + extraData + ); + } + + /// @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. + /// @param extraData The 32-byte deposit extra data. + /// @dev This function is extracted to overcome the stack too deep error. + function _emitDepositRevealedEvent( + bytes32 fundingTxHash, + uint64 fundingOutputAmount, + DepositRevealInfo calldata reveal, + bytes32 extraData + ) internal { // slither-disable-next-line reentrancy-events emit DepositRevealed( fundingTxHash, @@ -332,7 +353,7 @@ library Deposit { // reveal flow and reduce potential attack surface. require(extraData != bytes32(0), "Extra data must not be empty"); - revealDeposit(self, fundingTx, reveal, extraData); + _revealDeposit(self, fundingTx, reveal, extraData); } /// @notice Validates the deposit refund locktime. The validation passes From 182988db987e4d22a44b26e1b01c20ae684f2eba Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Mon, 18 Dec 2023 18:17:40 +0100 Subject: [PATCH 3/9] Store `extraData` as part of `DepositRequest` struct It turns out that emitting `extraData` as part of the existing `DepositRevealed` event is problematic as it is not compatible backward. Adding a new field to the event changes its signature (it is computed as keccak256 of the event name and argument types) so off-chain clients will not be able to fetch past `DepositRevealed` events (without the `extraData` field) using the new signature of the `DepositRevealed` (with added `extraData` field) event. --- solidity/contracts/bridge/Bridge.sol | 3 +-- solidity/contracts/bridge/Deposit.sol | 20 +++++++------------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/solidity/contracts/bridge/Bridge.sol b/solidity/contracts/bridge/Bridge.sol index 8692e1988..8f9d699b1 100644 --- a/solidity/contracts/bridge/Bridge.sol +++ b/solidity/contracts/bridge/Bridge.sol @@ -82,8 +82,7 @@ contract Bridge is bytes20 indexed walletPubKeyHash, bytes20 refundPubKeyHash, bytes4 refundLocktime, - address vault, - bytes32 extraData + address vault ); event DepositsSwept(bytes20 walletPubKeyHash, bytes32 sweepTxHash); diff --git a/solidity/contracts/bridge/Deposit.sol b/solidity/contracts/bridge/Deposit.sol index 11370c5e5..a8991e7bd 100644 --- a/solidity/contracts/bridge/Deposit.sol +++ b/solidity/contracts/bridge/Deposit.sol @@ -96,6 +96,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. @@ -110,8 +112,7 @@ library Deposit { bytes20 indexed walletPubKeyHash, bytes20 refundPubKeyHash, bytes4 refundLocktime, - address vault, - bytes32 extraData + address vault ); /// @notice Used by the depositor to reveal information about their P2(W)SH @@ -306,26 +307,20 @@ library Deposit { deposit.treasuryFee = self.depositTreasuryFeeDivisor > 0 ? fundingOutputAmount / self.depositTreasuryFeeDivisor : 0; + deposit.extraData = extraData; - _emitDepositRevealedEvent( - fundingTxHash, - fundingOutputAmount, - reveal, - 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. - /// @param extraData The 32-byte deposit extra data. /// @dev This function is extracted to overcome the stack too deep error. function _emitDepositRevealedEvent( bytes32 fundingTxHash, uint64 fundingOutputAmount, - DepositRevealInfo calldata reveal, - bytes32 extraData + DepositRevealInfo calldata reveal ) internal { // slither-disable-next-line reentrancy-events emit DepositRevealed( @@ -337,8 +332,7 @@ library Deposit { reveal.walletPubKeyHash, reveal.refundPubKeyHash, reveal.refundLocktime, - reveal.vault, - extraData + reveal.vault ); } From 481d0201b28931700ad394a578f09d9b71552966 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Mon, 18 Dec 2023 19:47:32 +0100 Subject: [PATCH 4/9] Add documentation to the recent changes around `Bridge` and extra data --- solidity/contracts/bridge/Bridge.sol | 22 ++++++++++- solidity/contracts/bridge/Deposit.sol | 55 ++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/solidity/contracts/bridge/Bridge.sol b/solidity/contracts/bridge/Bridge.sol index 8f9d699b1..f1395e04e 100644 --- a/solidity/contracts/bridge/Bridge.sol +++ b/solidity/contracts/bridge/Bridge.sol @@ -376,7 +376,27 @@ contract Bridge is self.revealDeposit(fundingTx, reveal); } - // TODO: Documentation and tests. + /// @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( BitcoinTx.Info calldata fundingTx, Deposit.DepositRevealInfo calldata reveal, diff --git a/solidity/contracts/bridge/Deposit.sol b/solidity/contracts/bridge/Deposit.sol index a8991e7bd..a0b2d9d3e 100644 --- a/solidity/contracts/bridge/Deposit.sol +++ b/solidity/contracts/bridge/Deposit.sol @@ -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: +/// +/// ``` +/// DROP +/// DROP +/// DROP +/// DUP HASH160 EQUAL +/// IF +/// CHECKSIG +/// ELSE +/// DUP HASH160 EQUALVERIFY +/// CHECKLOCKTIMEVERIFY DROP +/// CHECKSIG +/// ENDIF +/// ``` library Deposit { using BTCUtils for bytes; using BytesLib for bytes; @@ -157,7 +176,19 @@ library Deposit { _revealDeposit(self, fundingTx, reveal, bytes32(0)); } - // TODO: Documentation and think about better name. + /// @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, @@ -336,7 +367,27 @@ library Deposit { ); } - // TODO: Documentation and tests. + /// @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( BridgeState.Storage storage self, BitcoinTx.Info calldata fundingTx, From dfe6d886ea3b7679de9866814f7d2aa07179a797 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 19 Dec 2023 14:17:58 +0100 Subject: [PATCH 5/9] Adjust `WalletProposalValidator` to properly support deposits with extra data A wallet that is about to sweep some deposits calls the `WalletProposalValidator.validateDepositSweepProposal` to ensure the received sweep proposal is valid. This function must be aware of deposits with extra data as it checks deposit scripts. Here, we are ensuring that. --- .../bridge/WalletProposalValidator.sol | 94 +++++++++++++------ 1 file changed, 67 insertions(+), 27 deletions(-) diff --git a/solidity/contracts/bridge/WalletProposalValidator.sol b/solidity/contracts/bridge/WalletProposalValidator.sol index 4a0dddb3d..87c8b429f 100644 --- a/solidity/contracts/bridge/WalletProposalValidator.sol +++ b/solidity/contracts/bridge/WalletProposalValidator.sol @@ -274,6 +274,7 @@ contract WalletProposalValidator { validateDepositExtraInfo( depositKey, depositRequest.depositor, + depositRequest.extraData, depositExtraInfo ); @@ -357,6 +358,7 @@ contract WalletProposalValidator { /// is heavily based on `Deposit.revealDeposit` function. /// @param depositKey Key of the given deposit. /// @param depositor Depositor that revealed the deposit. + /// @param extraData 32-byte deposit extra data. Optional, can be bytes32(0). /// @param depositExtraInfo Extra data being subject of the validation. /// @dev Requirements: /// - The transaction hash computed using `depositExtraInfo.fundingTx` @@ -372,6 +374,7 @@ contract WalletProposalValidator { function validateDepositExtraInfo( DepositKey memory depositKey, address depositor, + bytes32 extraData, DepositExtraInfo memory depositExtraInfo ) internal view { bytes32 depositExtraFundingTxHash = abi @@ -389,33 +392,70 @@ contract WalletProposalValidator { revert("Extra info funding tx hash does not match"); } - bytes memory expectedScript = abi.encodePacked( - hex"14", // Byte length of depositor Ethereum address. - depositor, - hex"75", // OP_DROP - hex"08", // Byte length of blinding factor value. - depositExtraInfo.blindingFactor, - hex"75", // OP_DROP - hex"76", // OP_DUP - hex"a9", // OP_HASH160 - hex"14", // Byte length of a compressed Bitcoin public key hash. - depositExtraInfo.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. - depositExtraInfo.refundPubKeyHash, - hex"88", // OP_EQUALVERIFY - hex"04", // Byte length of refund locktime value. - depositExtraInfo.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. + depositor, + hex"75", // OP_DROP + hex"08", // Byte length of blinding factor value. + depositExtraInfo.blindingFactor, + hex"75", // OP_DROP + hex"76", // OP_DUP + hex"a9", // OP_HASH160 + hex"14", // Byte length of a compressed Bitcoin public key hash. + depositExtraInfo.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. + depositExtraInfo.refundPubKeyHash, + hex"88", // OP_EQUALVERIFY + hex"04", // Byte length of refund locktime value. + depositExtraInfo.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. + depositor, + hex"75", // OP_DROP + hex"20", // Byte length of extra data. + extraData, + hex"75", // OP_DROP + hex"08", // Byte length of blinding factor value. + depositExtraInfo.blindingFactor, + hex"75", // OP_DROP + hex"76", // OP_DUP + hex"a9", // OP_HASH160 + hex"14", // Byte length of a compressed Bitcoin public key hash. + depositExtraInfo.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. + depositExtraInfo.refundPubKeyHash, + hex"88", // OP_EQUALVERIFY + hex"04", // Byte length of refund locktime value. + depositExtraInfo.refundLocktime, + hex"b1", // OP_CHECKLOCKTIMEVERIFY + hex"75", // OP_DROP + hex"ac", // OP_CHECKSIG + hex"68" // OP_ENDIF + ); + } bytes memory fundingOutput = depositExtraInfo .fundingTx From d12b0e1d95d460535e12bb3e926a35e76977ab63 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 19 Dec 2023 16:37:29 +0100 Subject: [PATCH 6/9] Adjust failing unit tests --- solidity/test/bridge/WalletProposalValidator.test.ts | 1 + solidity/test/vault/TBTCVault.OptimisticMinting.test.ts | 3 +++ 2 files changed, 4 insertions(+) diff --git a/solidity/test/bridge/WalletProposalValidator.test.ts b/solidity/test/bridge/WalletProposalValidator.test.ts index 8755e057d..d238d2c58 100644 --- a/solidity/test/bridge/WalletProposalValidator.test.ts +++ b/solidity/test/bridge/WalletProposalValidator.test.ts @@ -2010,6 +2010,7 @@ const createTestDeposit = ( vault, treasuryFee: 0, // not relevant sweptAt: 0, // important to pass the validation + extraData: ethers.constants.HashZero, // not relevant }, extraInfo: { fundingTx, diff --git a/solidity/test/vault/TBTCVault.OptimisticMinting.test.ts b/solidity/test/vault/TBTCVault.OptimisticMinting.test.ts index d09356777..717f1b8ef 100644 --- a/solidity/test/vault/TBTCVault.OptimisticMinting.test.ts +++ b/solidity/test/vault/TBTCVault.OptimisticMinting.test.ts @@ -1789,6 +1789,7 @@ describe("TBTCVault - OptimisticMinting", () => { vault: f.tbtcVault.address, treasuryFee: 10, sweptAt: 0, + extraData: ethers.constants.HashZero, }) f.mockBridge.deposits.whenCalledWith(secondDepositID).returns({ depositor: depositorAddress, @@ -1797,6 +1798,7 @@ describe("TBTCVault - OptimisticMinting", () => { vault: f.tbtcVault.address, treasuryFee: 15, sweptAt: 0, + extraData: ethers.constants.HashZero, }) await f.tbtcVault @@ -1895,6 +1897,7 @@ describe("TBTCVault - OptimisticMinting", () => { vault: f.tbtcVault.address, treasuryFee: 10, sweptAt: 0, + extraData: ethers.constants.HashZero, }) await f.tbtcVault From 4ad2b60d63f43513d8cbdef9e8ab45c5affd9949 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 20 Dec 2023 18:04:25 +0100 Subject: [PATCH 7/9] Unit tests covering the `revealDepositWithExtraData` function Here we provide unit tests covering the new `revealDepositWithExtraData` function. Those tests are heavily based on the existing `revealDeposit` tests. Moreover, we are improving unit tests for `revealDeposit` to cover new cases that emerge due to the addition of `revealDepositWithExtraData`. --- solidity/test/bridge/Bridge.Deposit.test.ts | 978 +++++++++++++++++++- 1 file changed, 962 insertions(+), 16 deletions(-) diff --git a/solidity/test/bridge/Bridge.Deposit.test.ts b/solidity/test/bridge/Bridge.Deposit.test.ts index 218f0b126..a15799eed 100644 --- a/solidity/test/bridge/Bridge.Deposit.test.ts +++ b/solidity/test/bridge/Bridge.Deposit.test.ts @@ -16,7 +16,10 @@ import type { IVault, BridgeGovernance, } from "../../typechain" -import type { DepositRevealInfoStruct } from "../../typechain/Bridge" +import type { + DepositRevealInfoStruct, + InfoStruct as BitcoinTxInfoStruct, +} from "../../typechain/Bridge" import bridgeFixture from "../fixtures/bridge" import { constants, walletState } from "../fixtures" import { @@ -74,12 +77,19 @@ describe("Bridge - Deposit", () => { await bridge.setDepositRevealAheadPeriod(0) }) - describe("revealDeposit", () => { + type RevealDepositFixture = { + P2SHFundingTx: BitcoinTxInfoStruct + P2WSHFundingTx: BitcoinTxInfoStruct + depositorAddress: string + reveal: DepositRevealInfoStruct + extraData?: string + } + + // Fixture used for revealDeposit test scenario. + const revealDepositFixture: RevealDepositFixture = { // Data of a proper P2SH deposit funding transaction. Little-endian hash is: - // 0x17350f81cdb61cd8d7014ad1507d4af8d032b75812cf88d2c636c1c022991af2 and - // this is the same as `expectedP2SHDeposit.transaction` mentioned in - // tbtc-ts/test/deposit.test.ts file. - const P2SHFundingTx = { + // 0x17350f81cdb61cd8d7014ad1507d4af8d032b75812cf88d2c636c1c022991af2 + P2SHFundingTx: { version: "0x01000000", inputVector: "0x018348cdeb551134fe1f19d378a8adec9b146671cb67b945b71bf56b20d" + @@ -89,13 +99,10 @@ describe("Bridge - Deposit", () => { "da455877ed73b00000000001600147ac2d9378a1c47e589dfb8095ca95ed2" + "140d2726", locktime: "0x00000000", - } - + }, // Data of a proper P2WSH deposit funding transaction. Little-endian hash is: - // 0x6a81de17ce3da1eadc833c5fd9d85dac307d3b78235f57afbcd9f068fc01b99e and - // this is the same as `expectedP2WSHDeposit.transaction` mentioned in - // tbtc-ts/test/deposit.test.ts file. - const P2WSHFundingTx = { + // 0x6a81de17ce3da1eadc833c5fd9d85dac307d3b78235f57afbcd9f068fc01b99e. + P2WSHFundingTx: { version: "0x01000000", inputVector: "0x018348cdeb551134fe1f19d378a8adec9b146671cb67b945b71bf56b20d" + @@ -105,12 +112,56 @@ describe("Bridge - Deposit", () => { "b87d2b6a37d6c3b64722be83c636f10d73b00000000001600147ac2d9378a" + "1c47e589dfb8095ca95ed2140d2726", locktime: "0x00000000", - } + }, + // Data matching the redeem script locking the funding output of + // P2SHFundingTx and P2WSHFundingTx. + depositorAddress: "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637", + reveal: { + fundingOutputIndex: 0, + blindingFactor: "0xf9f0c90d00039523", + // HASH160 of 03989d253b17a6a0f41838b84ff0d20e8898f9d7b1a98f2564da4cc29dcf8581d9. + walletPubKeyHash: "0x8db50eb52063ea9d98b3eac91489a90f738986f6", + // HASH160 of 0300d6f28a2f6bf9836f57fcda5d284c9a8f849316119779f0d6090830d97763a9. + refundPubKeyHash: "0x28e081f285138ccbe389c1eb8985716230129f89", + refundLocktime: "0x60bcea61", + vault: "0x594cfd89700040163727828AE20B52099C58F02C", + }, + } + // Fixture used for revealDepositWithExtraData test scenario. + const revealDepositWithExtraDataFixture: RevealDepositFixture = { + // Data of a proper P2SH deposit funding transaction embedding some + // extra data. Little-endian hash is: + // 0x6383cd1829260b6034cd12bad36171748e8c3c6a8d57fcb6463c62f96116dfbc. + P2SHFundingTx: { + version: "0x01000000", + inputVector: + "0x018348cdeb551134fe1f19d378a8adec9b146671cb67b945b71bf56b20d" + + "c2b952f0100000000ffffffff", + outputVector: + "0x02102700000000000017a9149fe6615a307aa1d7eee668c1227802b2fbc" + + "aa919877ed73b00000000001600147ac2d9378a1c47e589dfb8095ca95ed2" + + "140d2726", + locktime: "0x00000000", + }, + // Data of a proper P2WSH deposit funding transaction embedding some + // extra data. Little-endian hash is: + // 0xc9312103d0d8d55344ef2d51acc409e004fbaaba7893b1725fa505ff73795732. + P2WSHFundingTx: { + version: "0x01000000", + inputVector: + "0x018348cdeb551134fe1f19d378a8adec9b146671cb67b945b71bf56b20d" + + "c2b952f0100000000ffffffff", + outputVector: + "0x021027000000000000220020bfaeddba12b0de6feeb649af76376876bc1" + + "feb6c2248fbfef9293ba3ac51bb4a10d73b00000000001600147ac2d9378a" + + "1c47e589dfb8095ca95ed2140d2726", + locktime: "0x00000000", + }, // Data matching the redeem script locking the funding output of // P2SHFundingTx and P2WSHFundingTx. - const depositorAddress = "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637" - const reveal = { + depositorAddress: "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637", + reveal: { fundingOutputIndex: 0, blindingFactor: "0xf9f0c90d00039523", // HASH160 of 03989d253b17a6a0f41838b84ff0d20e8898f9d7b1a98f2564da4cc29dcf8581d9. @@ -119,7 +170,15 @@ describe("Bridge - Deposit", () => { refundPubKeyHash: "0x28e081f285138ccbe389c1eb8985716230129f89", refundLocktime: "0x60bcea61", vault: "0x594cfd89700040163727828AE20B52099C58F02C", - } + }, + // sha256("fancy extra data") + extraData: + "0xa9b38ea6435c8941d6eda6a46b68e3e2117196995bd154ab55196396b03d9bda", + } + + describe("revealDeposit", () => { + const { P2SHFundingTx, P2WSHFundingTx, depositorAddress, reveal } = + revealDepositFixture let depositor: SignerWithAddress @@ -208,6 +267,10 @@ describe("Bridge - Deposit", () => { expect(deposit.treasuryFee).to.be.equal(5) // Swept time should be unset. expect(deposit.sweptAt).to.be.equal(0) + // Extra data must not be set. + expect(deposit.extraData).to.be.equal( + ethers.constants.HashZero + ) }) it("should emit DepositRevealed event", async () => { @@ -306,6 +369,11 @@ describe("Bridge - Deposit", () => { // value of the `depositTreasuryFeeDivisor`. // The divisor is 0 so the treasury fee is 0 as well. expect(deposit.treasuryFee).to.be.equal(0) + + // Extra data must not be set. + expect(deposit.extraData).to.be.equal( + ethers.constants.HashZero + ) }) it("should accept the deposit", async () => { @@ -425,6 +493,18 @@ describe("Bridge - Deposit", () => { }) } ) + + context("when funding transaction embeds extra data", () => { + it("should revert", async () => { + await expect( + bridge.connect(depositor).revealDeposit( + // Use a transaction that embeds extra data in the deposit script. + revealDepositWithExtraDataFixture.P2SHFundingTx, + reveal + ) + ).to.be.revertedWith("Wrong 20-byte script hash") + }) + }) }) context("when funding transaction is P2WSH", () => { @@ -476,6 +556,10 @@ describe("Bridge - Deposit", () => { expect(deposit.treasuryFee).to.be.equal(5) // Swept time should be unset. expect(deposit.sweptAt).to.be.equal(0) + // Extra data must not be set. + expect(deposit.extraData).to.be.equal( + ethers.constants.HashZero + ) }) it("should emit DepositRevealed event", async () => { @@ -607,6 +691,18 @@ describe("Bridge - Deposit", () => { }) } ) + + context("when funding transaction embeds extra data", () => { + it("should revert", async () => { + await expect( + bridge.connect(depositor).revealDeposit( + // Use a transaction that embeds extra data in the deposit script. + revealDepositWithExtraDataFixture.P2WSHFundingTx, + reveal + ) + ).to.be.revertedWith("Wrong 32-byte script hash") + }) + }) }) context("when funding transaction is neither P2SH nor P2WSH", () => { @@ -781,6 +877,856 @@ describe("Bridge - Deposit", () => { }) }) + describe("revealDepositWithExtraData", () => { + const { + P2SHFundingTx, + P2WSHFundingTx, + depositorAddress, + reveal, + extraData, + } = revealDepositWithExtraDataFixture + + let depositor: SignerWithAddress + + before(async () => { + depositor = await impersonateAccount(depositorAddress, { + from: governance, + value: 10, + }) + }) + + context("when extra data is non-zero", () => { + context("when wallet is in Live state", () => { + before(async () => { + await createSnapshot() + + await bridgeGovernance + .connect(governance) + .setVaultStatus(reveal.vault, true) + + // Simulate the wallet is a Live one and is known in the system. + await bridge.setWallet(reveal.walletPubKeyHash, { + ecdsaWalletID: ethers.constants.HashZero, + mainUtxoHash: ethers.constants.HashZero, + pendingRedemptionsValue: 0, + createdAt: await lastBlockTime(), + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.Live, + movingFundsTargetWalletsCommitmentHash: ethers.constants.HashZero, + }) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when reveal ahead period validation is disabled", () => { + context("when funding transaction is P2SH", () => { + context("when funding output script hash is correct", () => { + context("when deposit was not revealed yet", () => { + context("when amount is not below the dust threshold", () => { + context("when deposit is routed to a trusted vault", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + tx = await bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + reveal, + extraData + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should store proper deposit data", async () => { + // Deposit key is keccak256(fundingTxHash | fundingOutputIndex). + const depositKey = ethers.utils.solidityKeccak256( + ["bytes32", "uint32"], + [ + "0x6383cd1829260b6034cd12bad36171748e8c3c6a8d57fcb6463c62f96116dfbc", + reveal.fundingOutputIndex, + ] + ) + + const deposit = await bridge.deposits(depositKey) + + // Depositor address, same as in `reveal.depositor`. + expect(deposit.depositor).to.be.equal( + "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637" + ) + // Deposit amount in satoshi. In this case it's 10000 satoshi + // because the P2SH deposit transaction set this value for the + // funding output. + expect(deposit.amount).to.be.equal(10000) + // Revealed time should be set. + expect(deposit.revealedAt).to.be.equal( + await lastBlockTime() + ) + // Deposit vault, same as in `reveal.vault`. + expect(deposit.vault).to.be.equal( + "0x594cfd89700040163727828AE20B52099C58F02C" + ) + // Treasury fee should be computed according to the current + // value of the `depositTreasuryFeeDivisor`. + expect(deposit.treasuryFee).to.be.equal(5) + // Swept time should be unset. + expect(deposit.sweptAt).to.be.equal(0) + // Extra data must be set. + expect(deposit.extraData).to.be.equal(extraData) + }) + + it("should emit DepositRevealed event", async () => { + await expect(tx) + .to.emit(bridge, "DepositRevealed") + .withArgs( + "0x6383cd1829260b6034cd12bad36171748e8c3c6a8d57fcb6463c62f96116dfbc", + reveal.fundingOutputIndex, + "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637", + 10000, + "0xf9f0c90d00039523", + "0x8db50eb52063ea9d98b3eac91489a90f738986f6", + "0x28e081f285138ccbe389c1eb8985716230129f89", + "0x60bcea61", + reveal.vault + ) + }) + }) + + context("when deposit is not routed to a vault", () => { + let tx: ContractTransaction + let nonRoutedReveal: DepositRevealInfoStruct + + before(async () => { + await createSnapshot() + + nonRoutedReveal = { ...reveal } + nonRoutedReveal.vault = ZERO_ADDRESS + tx = await bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + nonRoutedReveal, + extraData + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should accept the deposit", async () => { + await expect(tx) + .to.emit(bridge, "DepositRevealed") + .withArgs( + "0x6383cd1829260b6034cd12bad36171748e8c3c6a8d57fcb6463c62f96116dfbc", + reveal.fundingOutputIndex, + "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637", + 10000, + "0xf9f0c90d00039523", + "0x8db50eb52063ea9d98b3eac91489a90f738986f6", + "0x28e081f285138ccbe389c1eb8985716230129f89", + "0x60bcea61", + ZERO_ADDRESS + ) + }) + }) + + context("when deposit treasury fee is zero", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + await bridgeGovernance + .connect(governance) + .beginDepositTreasuryFeeDivisorUpdate(0) + await helpers.time.increaseTime(constants.governanceDelay) + await bridgeGovernance + .connect(governance) + .finalizeDepositTreasuryFeeDivisorUpdate() + + tx = await bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + reveal, + extraData + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should store proper deposit data", async () => { + // Deposit key is keccak256(fundingTxHash | fundingOutputIndex). + const depositKey = ethers.utils.solidityKeccak256( + ["bytes32", "uint32"], + [ + "0x6383cd1829260b6034cd12bad36171748e8c3c6a8d57fcb6463c62f96116dfbc", + reveal.fundingOutputIndex, + ] + ) + + const deposit = await bridge.deposits(depositKey) + + // Deposit amount in satoshi. In this case it's 10000 satoshi + // because the P2SH deposit transaction set this value for the + // funding output. + expect(deposit.amount).to.be.equal(10000) + + // Treasury fee should be computed according to the current + // value of the `depositTreasuryFeeDivisor`. + // The divisor is 0 so the treasury fee is 0 as well. + expect(deposit.treasuryFee).to.be.equal(0) + // Extra data must be set. + expect(deposit.extraData).to.be.equal(extraData) + }) + + it("should accept the deposit", async () => { + await expect(tx) + .to.emit(bridge, "DepositRevealed") + .withArgs( + "0x6383cd1829260b6034cd12bad36171748e8c3c6a8d57fcb6463c62f96116dfbc", + reveal.fundingOutputIndex, + "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637", + 10000, + "0xf9f0c90d00039523", + "0x8db50eb52063ea9d98b3eac91489a90f738986f6", + "0x28e081f285138ccbe389c1eb8985716230129f89", + "0x60bcea61", + reveal.vault + ) + }) + }) + + context( + "when deposit is routed to a non-trusted vault", + () => { + let nonTrustedVaultReveal + + before(async () => { + await createSnapshot() + + nonTrustedVaultReveal = { ...reveal } + nonTrustedVaultReveal.vault = + "0x92499afEAD6c41f757Ec3558D0f84bf7ec5aD967" + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + nonTrustedVaultReveal, + extraData + ) + ).to.be.revertedWith("Vault is not trusted") + }) + } + ) + }) + + context("when amount is below the dust threshold", () => { + before(async () => { + await createSnapshot() + + // The `P2SHFundingTx` used within this scenario has an output + // whose value is 10000 satoshi. To make the scenario happen, it + // is enough that the contract's deposit dust threshold is + // bigger by 1 satoshi. + await bridge.setDepositDustThreshold(10001) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + reveal, + extraData + ) + ).to.be.revertedWith("Deposit amount too small") + }) + }) + }) + + context("when deposit was already revealed", () => { + before(async () => { + await createSnapshot() + + await bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + reveal, + extraData + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + reveal, + extraData + ) + ).to.be.revertedWith("Deposit already revealed") + }) + }) + }) + + context("when funding output script hash is wrong", () => { + it("should revert", async () => { + // Corrupt reveal data by setting a wrong blinding factor + const corruptedReveal = { ...reveal } + corruptedReveal.blindingFactor = "0xf9f0c90d00039524" + + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + corruptedReveal, + extraData + ) + ).to.be.revertedWith("Wrong 20-byte script hash") + }) + }) + + context( + "when the caller address does not match the funding output script", + () => { + it("should revert", async () => { + const accounts = await getUnnamedAccounts() + const thirdParty = await ethers.getSigner(accounts[0]) + + await expect( + bridge + .connect(thirdParty) + .revealDepositWithExtraData( + P2SHFundingTx, + reveal, + extraData + ) + ).to.be.revertedWith("Wrong 20-byte script hash") + }) + } + ) + + context("when the revealed extra data do not match", () => { + it("should revert", async () => { + // Corrupt the extra data. + const corruptedExtraData = ethers.utils.keccak256(extraData) + + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + reveal, + corruptedExtraData + ) + ).to.be.revertedWith("Wrong 20-byte script hash") + }) + }) + + context( + "when funding transaction does not embed extra data", + () => { + it("should revert", async () => { + await expect( + bridge.connect(depositor).revealDepositWithExtraData( + // Use a transaction that doesn't embed extra data in the deposit script. + revealDepositFixture.P2SHFundingTx, + reveal, + extraData + ) + ).to.be.revertedWith("Wrong 20-byte script hash") + }) + } + ) + }) + + context("when funding transaction is P2WSH", () => { + context("when funding output script hash is correct", () => { + context("when deposit was not revealed yet", () => { + context("when deposit is routed to a trusted vault", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + tx = await bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + reveal, + extraData + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should store proper deposit data", async () => { + // Deposit key is keccak256(fundingTxHash | fundingOutputIndex). + const depositKey = ethers.utils.solidityKeccak256( + ["bytes32", "uint32"], + [ + "0xc9312103d0d8d55344ef2d51acc409e004fbaaba7893b1725fa505ff73795732", + reveal.fundingOutputIndex, + ] + ) + + const deposit = await bridge.deposits(depositKey) + + // Depositor address, same as in `reveal.depositor`. + expect(deposit.depositor).to.be.equal( + "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637" + ) + // Deposit amount in satoshi. In this case it's 10000 satoshi + // because the P2SH deposit transaction set this value for the + // funding output. + expect(deposit.amount).to.be.equal(10000) + // Revealed time should be set. + expect(deposit.revealedAt).to.be.equal( + await lastBlockTime() + ) + // Deposit vault, same as in `reveal.vault`. + expect(deposit.vault).to.be.equal( + "0x594cfd89700040163727828AE20B52099C58F02C" + ) + // Treasury fee should be computed according to the current + // value of the `depositTreasuryFeeDivisor`. + expect(deposit.treasuryFee).to.be.equal(5) + // Swept time should be unset. + expect(deposit.sweptAt).to.be.equal(0) + // Extra data must be set. + expect(deposit.extraData).to.be.equal(extraData) + }) + + it("should emit DepositRevealed event", async () => { + await expect(tx) + .to.emit(bridge, "DepositRevealed") + .withArgs( + "0xc9312103d0d8d55344ef2d51acc409e004fbaaba7893b1725fa505ff73795732", + reveal.fundingOutputIndex, + "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637", + 10000, + "0xf9f0c90d00039523", + "0x8db50eb52063ea9d98b3eac91489a90f738986f6", + "0x28e081f285138ccbe389c1eb8985716230129f89", + "0x60bcea61", + reveal.vault + ) + }) + }) + + context("when deposit is not routed to a vault", () => { + let tx: ContractTransaction + let nonRoutedReveal: DepositRevealInfoStruct + + before(async () => { + await createSnapshot() + + nonRoutedReveal = { ...reveal } + nonRoutedReveal.vault = ZERO_ADDRESS + tx = await bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + nonRoutedReveal, + extraData + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should accept the deposit", async () => { + await expect(tx) + .to.emit(bridge, "DepositRevealed") + .withArgs( + "0xc9312103d0d8d55344ef2d51acc409e004fbaaba7893b1725fa505ff73795732", + reveal.fundingOutputIndex, + "0x934B98637cA318a4D6E7CA6ffd1690b8e77df637", + 10000, + "0xf9f0c90d00039523", + "0x8db50eb52063ea9d98b3eac91489a90f738986f6", + "0x28e081f285138ccbe389c1eb8985716230129f89", + "0x60bcea61", + ZERO_ADDRESS + ) + }) + }) + + context("when deposit is routed to a non-trusted vault", () => { + let nonTrustedVaultReveal + + before(async () => { + await createSnapshot() + + nonTrustedVaultReveal = { ...reveal } + nonTrustedVaultReveal.vault = + "0x92499afEAD6c41f757Ec3558D0f84bf7ec5aD967" + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + nonTrustedVaultReveal, + extraData + ) + ).to.be.revertedWith("Vault is not trusted") + }) + }) + }) + + context("when deposit was already revealed", () => { + before(async () => { + await createSnapshot() + + await bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + reveal, + extraData + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + reveal, + extraData + ) + ).to.be.revertedWith("Deposit already revealed") + }) + }) + }) + + context("when funding output script hash is wrong", () => { + it("should revert", async () => { + // Corrupt reveal data by setting a wrong blinding factor + const corruptedReveal = { ...reveal } + corruptedReveal.blindingFactor = "0xf9f0c90d00039524" + + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + corruptedReveal, + extraData + ) + ).to.be.revertedWith("Wrong 32-byte script hash") + }) + }) + + context( + "when the caller address does not match the funding output script", + () => { + it("should revert", async () => { + const accounts = await getUnnamedAccounts() + const thirdParty = await ethers.getSigner(accounts[0]) + + await expect( + bridge + .connect(thirdParty) + .revealDepositWithExtraData( + P2WSHFundingTx, + reveal, + extraData + ) + ).to.be.revertedWith("Wrong 32-byte script hash") + }) + } + ) + + context("when the revealed extra data do not match", () => { + it("should revert", async () => { + // Corrupt the extra data. + const corruptedExtraData = ethers.utils.keccak256(extraData) + + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + reveal, + corruptedExtraData + ) + ).to.be.revertedWith("Wrong 32-byte script hash") + }) + }) + + context( + "when funding transaction does not embed extra data", + () => { + it("should revert", async () => { + await expect( + bridge.connect(depositor).revealDepositWithExtraData( + // Use a transaction that doesn't embed extra data in the deposit script. + revealDepositFixture.P2WSHFundingTx, + reveal, + extraData + ) + ).to.be.revertedWith("Wrong 32-byte script hash") + }) + } + ) + }) + + context("when funding transaction is neither P2SH nor P2WSH", () => { + it("should revert", async () => { + // Corrupt transaction output data by making a 21-byte script hash. + const corruptedP2SHFundingTx = { ...P2SHFundingTx } + corruptedP2SHFundingTx.outputVector = + "0x02102700000000000017a9156a6ade1c799a3e5a59678e776f21be14d66dc" + + "15ed8877ed73b00000000001600147ac2d9378a1c47e589dfb8095ca95ed2" + + "140d2726" + + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + corruptedP2SHFundingTx, + reveal, + extraData + ) + ).to.be.revertedWith("Wrong script hash length") + }) + }) + }) + + context("when reveal ahead period validation is enabled", () => { + const encodeRefundLocktime = (refundLocktimeTimestamp: number) => { + const refundLocktimeTimestampHex = BigNumber.from( + refundLocktimeTimestamp + ) + .toHexString() + .substring(2) + const refundLocktimeBuffer = Buffer.from( + refundLocktimeTimestampHex, + "hex" + ) + return `0x${refundLocktimeBuffer.reverse().toString("hex")}` + } + + before(async () => { + await createSnapshot() + + // Reveal ahead period is disabled by default in this test suite + // (see root before clause). We need to enable it manually. + await bridge.setDepositRevealAheadPeriod( + constants.depositRevealAheadPeriod + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when reveal ahead period is preserved", () => { + it("should pass the refund locktime validation", async () => { + const now = Math.floor(Date.now() / 1000) + const refundLocktimeDuration = 2592000 // 30 days + const refundLocktimeTimestamp = now + refundLocktimeDuration + const latestPossibleRevealTimestamp = + refundLocktimeTimestamp - constants.depositRevealAheadPeriod + + const alteredReveal = { + ...reveal, + refundLocktime: encodeRefundLocktime(refundLocktimeTimestamp), + } + + await ethers.provider.send("evm_setNextBlockTimestamp", [ + BigNumber.from(latestPossibleRevealTimestamp).toHexString(), + ]) + + // We cannot assert that the reveal transaction succeeded since + // we modified the revealed refund locktime which differs from + // the one embedded in the transaction P2SH. We just make sure + // the execution does not revert on the refund locktime validation. + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + alteredReveal, + extraData + ) + ).to.be.not.revertedWith("Deposit refund locktime is too close") + }) + }) + + context("when reveal ahead period is not preserved", () => { + it("should revert", async () => { + const now = Math.floor(Date.now() / 1000) + const refundLocktimeDuration = 2592000 // 30 days + const refundLocktimeTimestamp = now + refundLocktimeDuration + const latestPossibleRevealTimestamp = + refundLocktimeTimestamp - constants.depositRevealAheadPeriod + + const alteredReveal = { + ...reveal, + refundLocktime: encodeRefundLocktime(refundLocktimeTimestamp), + } + + await ethers.provider.send("evm_setNextBlockTimestamp", [ + BigNumber.from(latestPossibleRevealTimestamp + 1).toHexString(), + ]) + + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + alteredReveal, + extraData + ) + ).to.be.revertedWith("Deposit refund locktime is too close") + }) + }) + + context( + "when refund locktime integer value is less than 500M", + () => { + it("should revert", async () => { + const alteredReveal = { + ...reveal, + refundLocktime: encodeRefundLocktime(499999999), + } + + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2WSHFundingTx, + alteredReveal, + extraData + ) + ).to.be.revertedWith("Refund locktime must be a value >= 500M") + }) + } + ) + }) + }) + + context("when wallet is not in Live state", () => { + const testData = [ + { + testName: "when wallet state is Unknown", + walletState: walletState.Unknown, + }, + { + testName: "when wallet state is MovingFunds", + walletState: walletState.MovingFunds, + }, + { + testName: "when the source wallet is in the Closing state", + walletState: walletState.Closing, + }, + { + testName: "when wallet state is Closed", + walletState: walletState.Closed, + }, + { + testName: "when wallet state is Terminated", + walletState: walletState.Terminated, + }, + ] + + testData.forEach((test) => { + context(test.testName, () => { + before(async () => { + await createSnapshot() + await bridge.setWallet(reveal.walletPubKeyHash, { + ecdsaWalletID: ethers.constants.HashZero, + mainUtxoHash: ethers.constants.HashZero, + pendingRedemptionsValue: 0, + createdAt: await lastBlockTime(), + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: test.walletState, + movingFundsTargetWalletsCommitmentHash: + ethers.constants.HashZero, + }) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData(P2SHFundingTx, reveal, extraData) + ).to.be.revertedWith("Wallet must be in Live state") + }) + }) + }) + }) + }) + + context("when extra data is zero", () => { + it("should revert", async () => { + await expect( + bridge + .connect(depositor) + .revealDepositWithExtraData( + P2SHFundingTx, + reveal, + ethers.constants.HashZero + ) + ).to.be.revertedWith("Extra data must not be empty") + }) + }) + }) + describe("submitDepositSweepProof", () => { const walletDraft = { ecdsaWalletID: ethers.constants.HashZero, From 2a7d54be5d9f5c3d2f40c14b0999654bed479002 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Thu, 21 Dec 2023 11:31:33 +0100 Subject: [PATCH 8/9] Disambiguate the nomenclature within `WalletProposalValidator` contract The `WalletProposalValidator` contract uses a concept of `deposit extra info` which is an additional data required to perform validation of a deposit sweep proposal. However, most of the comments refer that as `deposit extra data` which is problematic in the light of the recently introduced 32-byte deposit extra data which is a different thing. To fix that, we are improving the applicable comments and refer to `deposit extra info` explicitly. --- .../bridge/WalletProposalValidator.sol | 22 +++++++++---------- .../bridge/WalletProposalValidator.test.ts | 18 +++++++-------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/solidity/contracts/bridge/WalletProposalValidator.sol b/solidity/contracts/bridge/WalletProposalValidator.sol index 87c8b429f..6d96765aa 100644 --- a/solidity/contracts/bridge/WalletProposalValidator.sol +++ b/solidity/contracts/bridge/WalletProposalValidator.sol @@ -65,7 +65,7 @@ contract WalletProposalValidator { uint32 fundingOutputIndex; } - /// @notice Helper structure holding deposit extra data required during + /// @notice Helper structure holding deposit extra info required during /// deposit sweep proposal validation. Basically, this structure /// is a combination of BitcoinTx.Info and relevant parts of /// Deposit.DepositRevealInfo. @@ -188,7 +188,7 @@ contract WalletProposalValidator { /// complexity. Instead of that, each off-chain wallet member is /// supposed to do that check on their own. /// @param proposal The sweeping proposal to validate. - /// @param depositsExtraInfo Deposits extra data required to perform the validation. + /// @param depositsExtraInfo Deposits extra info required to perform the validation. /// @return True if the proposal is valid. Reverts otherwise. /// @dev Requirements: /// - The target wallet must be in the Live state, @@ -196,7 +196,7 @@ contract WalletProposalValidator { /// the range [1, `DEPOSIT_SWEEP_MAX_SIZE`], /// - The length of `depositsExtraInfo` array must be equal to the /// length of `proposal.depositsKeys`, i.e. each deposit must - /// have exactly one set of corresponding extra data, + /// have exactly one set of corresponding extra info, /// - The proposed sweep tx fee must be grater than zero, /// - The proposed maximum per-deposit sweep tx fee must be lesser than /// or equal the maximum fee allowed by the Bridge (`Bridge.depositTxMaxFee`), @@ -204,7 +204,7 @@ contract WalletProposalValidator { /// - Each deposit must be old enough, i.e. at least `DEPOSIT_MIN_AGE /// elapsed since their reveal time, /// - Each deposit must not be swept yet, - /// - Each deposit must have valid extra data (see `validateDepositExtraInfo`), + /// - Each deposit must have valid extra info (see `validateDepositExtraInfo`), /// - Each deposit must have the refund safety margin preserved, /// - Each deposit must be controlled by the same wallet, /// - Each deposit must target the same vault, @@ -232,7 +232,7 @@ contract WalletProposalValidator { require( proposal.depositsKeys.length == depositsExtraInfo.length, - "Each deposit key must have matching extra data" + "Each deposit key must have matching extra info" ); validateSweepTxFee(proposal.sweepTxFee, proposal.depositsKeys.length); @@ -354,12 +354,12 @@ contract WalletProposalValidator { ); } - /// @notice Validates the extra data for the given deposit. This function + /// @notice Validates the extra info for the given deposit. This function /// is heavily based on `Deposit.revealDeposit` function. /// @param depositKey Key of the given deposit. /// @param depositor Depositor that revealed the deposit. /// @param extraData 32-byte deposit extra data. Optional, can be bytes32(0). - /// @param depositExtraInfo Extra data being subject of the validation. + /// @param depositExtraInfo Extra info being subject of the validation. /// @dev Requirements: /// - The transaction hash computed using `depositExtraInfo.fundingTx` /// must match the `depositKey.fundingTxHash`. This requirement @@ -369,7 +369,7 @@ contract WalletProposalValidator { /// - The P2(W)SH script inferred from `depositExtraInfo` is actually /// used to lock funds by the `depositKey.fundingOutputIndex` output /// of the `depositExtraInfo.fundingTx` transaction. This requirement - /// ensures the reveal data provided in the extra data container + /// ensures the reveal data provided in the extra info container /// actually matches the given deposit. function validateDepositExtraInfo( DepositKey memory depositKey, @@ -386,7 +386,7 @@ contract WalletProposalValidator { ) .hash256View(); - // Make sure the funding tx provided as part of deposit extra data + // Make sure the funding tx provided as part of deposit extra info // actually matches the deposit referred by the given deposit key. if (depositKey.fundingTxHash != depositExtraFundingTxHash) { revert("Extra info funding tx hash does not match"); @@ -463,7 +463,7 @@ contract WalletProposalValidator { .extractOutputAtIndex(depositKey.fundingOutputIndex); bytes memory fundingOutputHash = fundingOutput.extractHash(); - // Path that checks the deposit extra data validity in case the + // Path that checks the deposit extra info validity in case the // referred deposit is a P2SH. if ( // slither-disable-next-line calls-loop @@ -473,7 +473,7 @@ contract WalletProposalValidator { return; } - // Path that checks the deposit extra data validity in case the + // Path that checks the deposit extra info validity in case the // referred deposit is a P2WSH. if ( fundingOutputHash.length == 32 && diff --git a/solidity/test/bridge/WalletProposalValidator.test.ts b/solidity/test/bridge/WalletProposalValidator.test.ts index d238d2c58..735ad90e9 100644 --- a/solidity/test/bridge/WalletProposalValidator.test.ts +++ b/solidity/test/bridge/WalletProposalValidator.test.ts @@ -196,7 +196,7 @@ describe("WalletProposalValidator", () => { }) context("when sweep does not exceed the max size", () => { - context("when deposit extra data length does not match", () => { + context("when deposit extra info length does not match", () => { it("should revert", async () => { // The proposal contains one deposit. const proposal = { @@ -206,7 +206,7 @@ describe("WalletProposalValidator", () => { depositsRevealBlocks: [], // Not relevant in this scenario. } - // The extra data array contains two items. + // The extra info array contains two items. const depositsExtraInfo = [ emptyDepositExtraInfo, emptyDepositExtraInfo, @@ -218,12 +218,12 @@ describe("WalletProposalValidator", () => { depositsExtraInfo ) ).to.be.revertedWith( - "Each deposit key must have matching extra data" + "Each deposit key must have matching extra info" ) }) }) - context("when deposit extra data length matches", () => { + context("when deposit extra info length matches", () => { context("when proposed sweep tx fee is invalid", () => { context("when proposed sweep tx fee is zero", () => { let depositOne @@ -564,7 +564,7 @@ describe("WalletProposalValidator", () => { context("when all deposits are not swept yet", () => { context( - "when there is a deposit with invalid extra data", + "when there is a deposit with invalid extra info", () => { context("when funding tx hashes don't match", () => { let deposit @@ -602,7 +602,7 @@ describe("WalletProposalValidator", () => { depositsRevealBlocks: [], // Not relevant in this scenario. } - // Corrupt the extra data by setting a different + // Corrupt the extra info by setting a different // version than 0x01000000 used to produce the hash. const depositsExtraInfo = [ { @@ -663,7 +663,7 @@ describe("WalletProposalValidator", () => { depositsRevealBlocks: [], // Not relevant in this scenario. } - // Corrupt the extra data by reversing the proper + // Corrupt the extra info by reversing the proper // blinding factor used to produce the script. const depositsExtraInfo = [ { @@ -729,7 +729,7 @@ describe("WalletProposalValidator", () => { depositsRevealBlocks: [], // Not relevant in this scenario. } - // Corrupt the extra data by reversing the proper + // Corrupt the extra info by reversing the proper // blinding factor used to produce the script. const depositsExtraInfo = [ { @@ -759,7 +759,7 @@ describe("WalletProposalValidator", () => { } ) - context("when all deposits extra data are valid", () => { + context("when all deposits extra info are valid", () => { context( "when there is a deposit that violates the refund safety margin", () => { From 78dc18c9961d8e94a618428a998ff6fa86246255 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Thu, 21 Dec 2023 12:08:44 +0100 Subject: [PATCH 9/9] Unit tests covering changes in the `WalletProposalValidator` contract --- .../bridge/WalletProposalValidator.test.ts | 54 ++++++++++++++++--- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/solidity/test/bridge/WalletProposalValidator.test.ts b/solidity/test/bridge/WalletProposalValidator.test.ts index 735ad90e9..37b21651d 100644 --- a/solidity/test/bridge/WalletProposalValidator.test.ts +++ b/solidity/test/bridge/WalletProposalValidator.test.ts @@ -1116,6 +1116,7 @@ describe("WalletProposalValidator", () => { () => { let depositOne let depositTwo + let depositThree before(async () => { await createSnapshot() @@ -1132,6 +1133,16 @@ describe("WalletProposalValidator", () => { false ) + // Use a deposit with embedded 32-byte extra data + // to make sure validation handles them correctly. + depositThree = createTestDeposit( + walletPubKeyHash, + vault, + true, + undefined, + "0xa9b38ea6435c8941d6eda6a46b68e3e2117196995bd154ab55196396b03d9bda" + ) + bridge.deposits .whenCalledWith( depositKey( @@ -1149,6 +1160,16 @@ describe("WalletProposalValidator", () => { ) ) .returns(depositTwo.request) + + bridge.deposits + .whenCalledWith( + depositKey( + depositThree.key.fundingTxHash, + depositThree.key + .fundingOutputIndex + ) + ) + .returns(depositThree.request) }) after(async () => { @@ -1163,6 +1184,7 @@ describe("WalletProposalValidator", () => { depositsKeys: [ depositOne.key, depositTwo.key, + depositThree.key, ], sweepTxFee, depositsRevealBlocks: [], // Not relevant in this scenario. @@ -1171,6 +1193,7 @@ describe("WalletProposalValidator", () => { const depositsExtraInfo = [ depositOne.extraInfo, depositTwo.extraInfo, + depositThree.extraInfo, ] const result = @@ -1933,7 +1956,8 @@ const createTestDeposit = ( walletPubKeyHash: string, vault: string, witness = true, - revealedAt?: number + revealedAt?: number, + extraData?: string ) => { let resolvedRevealedAt = revealedAt @@ -1963,10 +1987,28 @@ const createTestDeposit = ( const blindingFactor = `0x${crypto.randomBytes(8).toString("hex")}` const refundPubKeyHash = `0x${crypto.randomBytes(20).toString("hex")}` - const depositScript = - `0x14${depositor.substring(2)}7508${blindingFactor.substring(2)}7576a914` + - `${walletPubKeyHash.substring(2)}8763ac6776a914` + - `${refundPubKeyHash.substring(2)}8804${refundLocktime.substring(2)}b175ac68` + let depositScript + + if (extraData) { + depositScript = + `0x14${depositor.substring(2)}75` + + `20${extraData.substring(2)}75` + + `08${blindingFactor.substring(2)}75` + + `76a914${walletPubKeyHash.substring(2)}87` + + "63ac67" + + `76a914${refundPubKeyHash.substring(2)}88` + + `04${refundLocktime.substring(2)}b175` + + "ac68" + } else { + depositScript = + `0x14${depositor.substring(2)}75` + + `08${blindingFactor.substring(2)}75` + + `76a914${walletPubKeyHash.substring(2)}87` + + "63ac67" + + `76a914${refundPubKeyHash.substring(2)}88` + + `04${refundLocktime.substring(2)}b175` + + "ac68" + } let depositScriptHash if (witness) { @@ -2010,7 +2052,7 @@ const createTestDeposit = ( vault, treasuryFee: 0, // not relevant sweptAt: 0, // important to pass the validation - extraData: ethers.constants.HashZero, // not relevant + extraData: extraData ?? ethers.constants.HashZero, }, extraInfo: { fundingTx,