From 5a021e8ba7bda6fbd2f33535ade6c2e8026e7cec Mon Sep 17 00:00:00 2001 From: Tomasz Slabon Date: Wed, 13 Dec 2023 11:20:11 +0100 Subject: [PATCH 1/3] Added validation of moving funds proposal --- .../bridge/WalletProposalValidator.sol | 156 ++++++++++++++++++ .../bridge/WalletProposalValidator.test.ts | 89 ++++++++++ 2 files changed, 245 insertions(+) diff --git a/solidity/contracts/bridge/WalletProposalValidator.sol b/solidity/contracts/bridge/WalletProposalValidator.sol index 4a0dddb3d..7793f2da7 100644 --- a/solidity/contracts/bridge/WalletProposalValidator.sol +++ b/solidity/contracts/bridge/WalletProposalValidator.sol @@ -94,6 +94,16 @@ contract WalletProposalValidator { uint256 redemptionTxFee; } + /// @notice Helper structure representing a moving funds proposal. + struct MovingFundsProposal { + // 20-byte public key hash of the source wallet. + bytes20 walletPubKeyHash; + // List of 20-byte public key hashes of target wallets. + bytes20[] targetWallets; + // Proposed BTC fee for the entire transaction. + uint256 movingFundsTxFee; + } + /// @notice Helper structure representing a heartbeat proposal. struct HeartbeatProposal { // 20-byte public key hash of the target wallet. @@ -587,6 +597,152 @@ contract WalletProposalValidator { return true; } + /// @notice View function encapsulating the main rules of a valid moving + /// funds proposal. This function is meant to facilitate the + /// off-chain validation of the incoming proposals. Thanks to it, + /// most of the work can be done using a single readonly contract + /// call. + /// @param proposal The moving funds proposal to validate. + /// @param walletMainUtxo The main UTXO of the source wallet. + /// @return True if the proposal is valid. Reverts otherwise. + /// @dev Requirements: + /// - The source wallet must be in the MovingFunds state, + /// - The source wallet must not have any pending redemptions, + /// - The source wallet must not have any pending moved funds sweep + /// requests, + /// - The source wallet's BTC balance must be greater than zero, + /// - The number of target wallets in the proposal must match the + /// expected count, based on the Live wallets count, source wallet's + /// BTC balance and maximum BTC transfer limit, + /// - All target wallets must be in the Live state and ordered correctly + /// (i.e. no duplicates and in ascending order of wallet addresses), + /// - The proposed moving funds transaction fee must be greater than + /// zero, + /// - The proposed moving funds transaction fee must not exceed the + /// maximum total fee allowed for moving funds. + function validateMovingFundsProposal( + MovingFundsProposal calldata proposal, + BitcoinTx.UTXO calldata walletMainUtxo + ) external view returns (bool) { + Wallets.Wallet memory sourceWallet = bridge.wallets( + proposal.walletPubKeyHash + ); + + // Make sure the source wallet is eligible for moving funds. + require( + sourceWallet.state == Wallets.WalletState.MovingFunds, + "Source wallet is not in MovingFunds state" + ); + + require( + sourceWallet.pendingRedemptionsValue == 0, + "Source wallet has pending redemptions" + ); + + require( + sourceWallet.pendingMovedFundsSweepRequestsCount == 0, + "Source wallet has pending moved funds sweep requests" + ); + + // Make sure the number of target wallets is correct. + uint64 sourceWalletBtcBalance = getWalletBtcBalance( + sourceWallet.mainUtxoHash, + walletMainUtxo + ); + + require( + sourceWalletBtcBalance > 0, + "Source wallet BTC balance is zero" + ); + + uint32 liveWalletsCount = bridge.liveWalletsCount(); + + (, , , , , uint64 walletMaxBtcTransfer, ) = bridge.walletParameters(); + + uint256 expectedTargetWalletsCount = Math.min( + liveWalletsCount, + Math.ceilDiv(sourceWalletBtcBalance, walletMaxBtcTransfer) + ); + + require(expectedTargetWalletsCount > 0, "No target wallets available"); + + require( + proposal.targetWallets.length == expectedTargetWalletsCount, + "Submitted target wallets count is other than expected" + ); + + // Make sure the target wallets are Live and are ordered correctly. + uint160 lastProcessedTargetWallet = 0; + + for (uint256 i = 0; i < proposal.targetWallets.length; i++) { + bytes20 targetWallet = proposal.targetWallets[i]; + + require( + targetWallet != proposal.walletPubKeyHash, + "Target wallet is equal to source wallet" + ); + + require( + uint160(targetWallet) > lastProcessedTargetWallet, + "Target wallet order is incorrect" + ); + + // slither-disable-next-line calls-loop + require( + bridge.wallets(targetWallet).state == Wallets.WalletState.Live, + "Target wallet is not in Live state" + ); + + lastProcessedTargetWallet = uint160(targetWallet); + } + + // Make sure the proposed fee does not exceed the total fee limit. + (uint64 movingFundsTxMaxTotalFee, , , , , , , , , , ) = bridge + .movingFundsParameters(); + + require( + proposal.movingFundsTxFee > 0, + "Proposed transaction fee cannot be zero" + ); + + require( + proposal.movingFundsTxFee <= movingFundsTxMaxTotalFee, + "Proposed transaction fee is too high" + ); + + return true; + } + + /// @notice Calculates the Bitcoin balance of a wallet based on its main + /// UTXO. + /// @param walletMainUtxoHash The hash of the wallet's main UTXO. + /// @param walletMainUtxo The detailed data of the wallet's main UTXO. + /// @return walletBtcBalance The calculated Bitcoin balance of the wallet. + function getWalletBtcBalance( + bytes32 walletMainUtxoHash, + BitcoinTx.UTXO calldata walletMainUtxo + ) internal view returns (uint64 walletBtcBalance) { + // If the wallet has a main UTXO hash set, cross-check it with the + // provided plain-text parameter and get the transaction output value + // as BTC balance. Otherwise, the BTC balance is just zero. + if (walletMainUtxoHash != bytes32(0)) { + require( + keccak256( + abi.encodePacked( + walletMainUtxo.txHash, + walletMainUtxo.txOutputIndex, + walletMainUtxo.txOutputValue + ) + ) == walletMainUtxoHash, + "Invalid wallet main UTXO data" + ); + + walletBtcBalance = walletMainUtxo.txOutputValue; + } + + return walletBtcBalance; + } + /// @notice View function encapsulating the main rules of a valid heartbeat /// proposal. This function is meant to facilitate the off-chain /// validation of the incoming proposals. Thanks to it, most diff --git a/solidity/test/bridge/WalletProposalValidator.test.ts b/solidity/test/bridge/WalletProposalValidator.test.ts index 8755e057d..fcd5c3d08 100644 --- a/solidity/test/bridge/WalletProposalValidator.test.ts +++ b/solidity/test/bridge/WalletProposalValidator.test.ts @@ -5,6 +5,7 @@ import { FakeContract, smock } from "@defi-wonderland/smock" import { BigNumber, BigNumberish, BytesLike } from "ethers" import type { Bridge, WalletProposalValidator } from "../../typechain" import { walletState } from "../fixtures" +import { NO_MAIN_UTXO } from "../data/deposit-sweep" chai.use(smock.matchers) @@ -1894,6 +1895,94 @@ describe("WalletProposalValidator", () => { }) }) + describe("validateMovingFundsProposal", () => { + const walletPubKeyHash = "0x7ac2d9378a1c47e589dfb8095ca95ed2140d2726" + const ecdsaWalletID = + "0x4ad6b3ccbca81645865d8d0d575797a15528e98ced22f29a6f906d3259569863" + + before(async () => { + await createSnapshot() + + // TODO: Fill with appropriate parameters. + bridge.movingFundsParameters.returns([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) + }) + + after(async () => { + bridge.movingFundsParameters.reset() + + await restoreSnapshot() + }) + + context("when wallet's state is not MovingFunds", () => { + const testData = [ + { + testName: "when wallet state is Unknown", + walletState: walletState.Unknown, + }, + { + testName: "when wallet state is Live", + walletState: walletState.Live, + }, + { + testName: "when wallet state is Closing", + 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() + + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID, + mainUtxoHash: HashZero, + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: test.walletState, + movingFundsTargetWalletsCommitmentHash: HashZero, + }) + }) + + after(async () => { + bridge.wallets.reset() + + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + // Only walletPubKeyHash argument is relevant in this scenario. + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }, + NO_MAIN_UTXO + ) + ).to.be.revertedWith("Source wallet is not in MovingFunds state") + }) + }) + }) + }) + + context("when wallet's state is MovingFunds", () => { + // TODO: Implement + }) + }) + describe("validateHeartbeatProposal", () => { context("when message is not valid", () => { it("should revert", async () => { From 6b58e868b783a83671bb9bb0f33255cf18f1ad66 Mon Sep 17 00:00:00 2001 From: Tomasz Slabon Date: Thu, 14 Dec 2023 15:23:49 +0100 Subject: [PATCH 2/3] Update unit tests --- .../bridge/WalletProposalValidator.test.ts | 491 +++++++++++++++++- 1 file changed, 488 insertions(+), 3 deletions(-) diff --git a/solidity/test/bridge/WalletProposalValidator.test.ts b/solidity/test/bridge/WalletProposalValidator.test.ts index fcd5c3d08..e00960d78 100644 --- a/solidity/test/bridge/WalletProposalValidator.test.ts +++ b/solidity/test/bridge/WalletProposalValidator.test.ts @@ -1899,16 +1899,31 @@ describe("WalletProposalValidator", () => { const walletPubKeyHash = "0x7ac2d9378a1c47e589dfb8095ca95ed2140d2726" const ecdsaWalletID = "0x4ad6b3ccbca81645865d8d0d575797a15528e98ced22f29a6f906d3259569863" + const movingFundsTxMaxTotalFee = 20000 + const walletMaxBtcTransfer = 30000000 before(async () => { await createSnapshot() - // TODO: Fill with appropriate parameters. - bridge.movingFundsParameters.returns([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) + bridge.movingFundsParameters.returns([ + movingFundsTxMaxTotalFee, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + ]) + bridge.walletParameters.returns([0, 0, 0, 0, 0, walletMaxBtcTransfer, 0]) }) after(async () => { bridge.movingFundsParameters.reset() + bridge.walletParameters.reset() await restoreSnapshot() }) @@ -1979,7 +1994,477 @@ describe("WalletProposalValidator", () => { }) context("when wallet's state is MovingFunds", () => { - // TODO: Implement + context("when the wallet has pending redemptions", () => { + before(async () => { + await createSnapshot() + + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID, + mainUtxoHash: HashZero, + pendingRedemptionsValue: 10000, // Make the value positive. + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.MovingFunds, + movingFundsTargetWalletsCommitmentHash: HashZero, + }) + }) + + after(async () => { + bridge.wallets.reset() + + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }, + NO_MAIN_UTXO + ) + ).to.be.revertedWith("Source wallet has pending redemptions") + }) + }) + + context("when the wallet does not have pending redemptions", () => { + context( + "when the wallet has pending moved funds sweep requests", + () => { + before(async () => { + await createSnapshot() + + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID, + mainUtxoHash: HashZero, + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 1, // Make the value positive. + state: walletState.MovingFunds, + movingFundsTargetWalletsCommitmentHash: HashZero, + }) + }) + + after(async () => { + bridge.wallets.reset() + + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }, + NO_MAIN_UTXO + ) + ).to.be.revertedWith( + "Source wallet has pending moved funds sweep requests" + ) + }) + } + ) + + context( + "when the wallet does not have pending moved funds sweep requests", + () => { + context("when the provided main UTXO is invalid", () => { + before(async () => { + await createSnapshot() + + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID, + mainUtxoHash: + // Use any non-zero hash to indicate the wallet has a main UTXO. + "0x1111111111111111111111111111111111111111111111111111111111111111", + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.MovingFunds, + movingFundsTargetWalletsCommitmentHash: HashZero, + }) + }) + + after(async () => { + bridge.wallets.reset() + + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 1000, + } + ) + ).to.be.revertedWith("Invalid wallet main UTXO data") + }) + }) + + context("when the provided main UTXO is valid", () => { + context("when the wallet's balance is zero", () => { + before(async () => { + await createSnapshot() + + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID, + // Use zero hash so that the wallet's main is considered not + // set. + mainUtxoHash: HashZero, + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.MovingFunds, + movingFundsTargetWalletsCommitmentHash: HashZero, + }) + }) + + after(async () => { + bridge.wallets.reset() + + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }, + NO_MAIN_UTXO + ) + ).to.be.revertedWith("Source wallet BTC balance is zero") + }) + }) + + context("when the wallet's balance is positive", () => { + const targetWallets = [ + "0x1111111111111111111111111111111111111111", + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + ] + + before(async () => { + await createSnapshot() + + // Set the source wallet. + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID, + mainUtxoHash: + "0x2cd9812b371968dc7e244c6757778d436cda256db583909a1758cac47c5d3df9", + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.MovingFunds, + movingFundsTargetWalletsCommitmentHash: HashZero, + }) + + // Set target wallets. + targetWallets.forEach((targetWallet) => { + bridge.wallets.whenCalledWith(targetWallet).returns({ + ecdsaWalletID, + mainUtxoHash: HashZero, + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.Live, + movingFundsTargetWalletsCommitmentHash: HashZero, + }) + }) + }) + + after(async () => { + bridge.wallets.reset() + + await restoreSnapshot() + }) + + context("when there are no target wallets available", () => { + before(async () => { + await createSnapshot() + + bridge.liveWalletsCount.returns(0) + }) + + after(async () => { + bridge.liveWalletsCount.reset() + + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 100000000, + } + ) + ).to.be.revertedWith("No target wallets available") + }) + }) + + context("when there are target wallets available", () => { + before(async () => { + await createSnapshot() + + bridge.liveWalletsCount.returns(3) + }) + + after(async () => { + bridge.liveWalletsCount.reset() + + await restoreSnapshot() + }) + + context( + "when the number of target wallets is other than expected", + () => { + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [ + // Provide one target wallet instead of three. + targetWallets[0], + ], + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 100000000, + } + ) + ).to.be.revertedWith( + "Submitted target wallets count is other than expected" + ) + }) + } + ) + + context( + "when the number of target wallets is expected", + () => { + context( + "when the source wallet is used as a target wallet", + () => { + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [ + "0x1111111111111111111111111111111111111111", + walletPubKeyHash, // Use source wallet. + "0x3333333333333333333333333333333333333333", + ], + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 100000000, + } + ) + ).to.be.revertedWith( + "Target wallet is equal to source wallet" + ) + }) + } + ) + + context( + "when the source wallet is not used as a target wallet", + () => { + context( + "when the order of target wallets is incorrect", + () => { + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [ + // Change order. + targetWallets[0], + targetWallets[2], + targetWallets[1], + ], + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 100000000, + } + ) + ).to.be.revertedWith( + "Target wallet order is incorrect" + ) + }) + } + ) + + context( + "when the order of target wallets is correct", + () => { + context( + "when one of the target wallets is not Live", + () => { + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [ + targetWallets[0], + targetWallets[1], + // Use unknown wallet, so that its state is not Unknown. + "0xcccccccccccccccccccccccccccccccccccccccc", + ], + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 100000000, + } + ) + ).to.be.revertedWith( + "Target wallet is not in Live state" + ) + }) + } + ) + + context( + "when all the target wallets are Live", + () => { + context( + "when the provided transaction fee is incorrect", + () => { + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets, + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 100000000, + } + ) + ).to.be.revertedWith( + "Proposed transaction fee cannot be zero" + ) + }) + + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: + movingFundsTxMaxTotalFee + 1, + targetWallets, + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 100000000, + } + ) + ).to.be.revertedWith( + "Proposed transaction fee is too high" + ) + }) + } + ) + + context( + "when the provided transaction fee is correct", + () => { + it("should pass validation", async () => { + const result = + await walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: + movingFundsTxMaxTotalFee, + targetWallets, + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 100000000, + } + ) + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect(result).to.be.true + }) + } + ) + } + ) + } + ) + } + ) + } + ) + }) + }) + }) + } + ) + }) }) }) From 3f7d6b3687df77a8402605f8575024dc95f2a33c Mon Sep 17 00:00:00 2001 From: Tomasz Slabon Date: Tue, 19 Dec 2023 15:08:20 +0100 Subject: [PATCH 3/3] Simplified moving funds proposal validation --- .../bridge/WalletProposalValidator.sol | 135 +---- .../bridge/WalletProposalValidator.test.ts | 548 ++++-------------- 2 files changed, 133 insertions(+), 550 deletions(-) diff --git a/solidity/contracts/bridge/WalletProposalValidator.sol b/solidity/contracts/bridge/WalletProposalValidator.sol index 7793f2da7..5163f3186 100644 --- a/solidity/contracts/bridge/WalletProposalValidator.sol +++ b/solidity/contracts/bridge/WalletProposalValidator.sol @@ -603,100 +603,49 @@ contract WalletProposalValidator { /// most of the work can be done using a single readonly contract /// call. /// @param proposal The moving funds proposal to validate. - /// @param walletMainUtxo The main UTXO of the source wallet. /// @return True if the proposal is valid. Reverts otherwise. - /// @dev Requirements: - /// - The source wallet must be in the MovingFunds state, - /// - The source wallet must not have any pending redemptions, - /// - The source wallet must not have any pending moved funds sweep - /// requests, - /// - The source wallet's BTC balance must be greater than zero, - /// - The number of target wallets in the proposal must match the - /// expected count, based on the Live wallets count, source wallet's - /// BTC balance and maximum BTC transfer limit, - /// - All target wallets must be in the Live state and ordered correctly - /// (i.e. no duplicates and in ascending order of wallet addresses), - /// - The proposed moving funds transaction fee must be greater than - /// zero, - /// - The proposed moving funds transaction fee must not exceed the - /// maximum total fee allowed for moving funds. - function validateMovingFundsProposal( - MovingFundsProposal calldata proposal, - BitcoinTx.UTXO calldata walletMainUtxo - ) external view returns (bool) { + /// @dev Notice that this function is meant to be invoked after the moving + /// funds commitment has already been submitted. This function skips + /// some checks related to the moving funds procedure as they were + /// already checked on the commitment submission. + /// Requirements: + /// - The source wallet must be in the MovingFunds state, + /// - The target wallets commitment must be submitted, + /// - The target wallets commitment hash must match the target wallets + /// from the proposal, + /// - The proposed moving funds transaction fee must be greater than + /// zero, + /// - The proposed moving funds transaction fee must not exceed the + /// maximum total fee allowed for moving funds. + function validateMovingFundsProposal(MovingFundsProposal calldata proposal) + external + view + returns (bool) + { Wallets.Wallet memory sourceWallet = bridge.wallets( proposal.walletPubKeyHash ); - // Make sure the source wallet is eligible for moving funds. + // Make sure the source wallet is in MovingFunds state. require( sourceWallet.state == Wallets.WalletState.MovingFunds, "Source wallet is not in MovingFunds state" ); + // Make sure the moving funds commitment has been submitted and + // the commitment hash matches the target wallets from the proposal. require( - sourceWallet.pendingRedemptionsValue == 0, - "Source wallet has pending redemptions" - ); - - require( - sourceWallet.pendingMovedFundsSweepRequestsCount == 0, - "Source wallet has pending moved funds sweep requests" - ); - - // Make sure the number of target wallets is correct. - uint64 sourceWalletBtcBalance = getWalletBtcBalance( - sourceWallet.mainUtxoHash, - walletMainUtxo - ); - - require( - sourceWalletBtcBalance > 0, - "Source wallet BTC balance is zero" + sourceWallet.movingFundsTargetWalletsCommitmentHash != bytes32(0), + "Target wallets commitment is not submitted" ); - uint32 liveWalletsCount = bridge.liveWalletsCount(); - - (, , , , , uint64 walletMaxBtcTransfer, ) = bridge.walletParameters(); - - uint256 expectedTargetWalletsCount = Math.min( - liveWalletsCount, - Math.ceilDiv(sourceWalletBtcBalance, walletMaxBtcTransfer) - ); - - require(expectedTargetWalletsCount > 0, "No target wallets available"); - require( - proposal.targetWallets.length == expectedTargetWalletsCount, - "Submitted target wallets count is other than expected" + sourceWallet.movingFundsTargetWalletsCommitmentHash == + keccak256(abi.encodePacked(proposal.targetWallets)), + "Target wallets do not match target wallets commitment hash" ); - // Make sure the target wallets are Live and are ordered correctly. - uint160 lastProcessedTargetWallet = 0; - - for (uint256 i = 0; i < proposal.targetWallets.length; i++) { - bytes20 targetWallet = proposal.targetWallets[i]; - - require( - targetWallet != proposal.walletPubKeyHash, - "Target wallet is equal to source wallet" - ); - - require( - uint160(targetWallet) > lastProcessedTargetWallet, - "Target wallet order is incorrect" - ); - - // slither-disable-next-line calls-loop - require( - bridge.wallets(targetWallet).state == Wallets.WalletState.Live, - "Target wallet is not in Live state" - ); - - lastProcessedTargetWallet = uint160(targetWallet); - } - - // Make sure the proposed fee does not exceed the total fee limit. + // Make sure the proposed fee is valid. (uint64 movingFundsTxMaxTotalFee, , , , , , , , , , ) = bridge .movingFundsParameters(); @@ -713,36 +662,6 @@ contract WalletProposalValidator { return true; } - /// @notice Calculates the Bitcoin balance of a wallet based on its main - /// UTXO. - /// @param walletMainUtxoHash The hash of the wallet's main UTXO. - /// @param walletMainUtxo The detailed data of the wallet's main UTXO. - /// @return walletBtcBalance The calculated Bitcoin balance of the wallet. - function getWalletBtcBalance( - bytes32 walletMainUtxoHash, - BitcoinTx.UTXO calldata walletMainUtxo - ) internal view returns (uint64 walletBtcBalance) { - // If the wallet has a main UTXO hash set, cross-check it with the - // provided plain-text parameter and get the transaction output value - // as BTC balance. Otherwise, the BTC balance is just zero. - if (walletMainUtxoHash != bytes32(0)) { - require( - keccak256( - abi.encodePacked( - walletMainUtxo.txHash, - walletMainUtxo.txOutputIndex, - walletMainUtxo.txOutputValue - ) - ) == walletMainUtxoHash, - "Invalid wallet main UTXO data" - ); - - walletBtcBalance = walletMainUtxo.txOutputValue; - } - - return walletBtcBalance; - } - /// @notice View function encapsulating the main rules of a valid heartbeat /// proposal. This function is meant to facilitate the off-chain /// validation of the incoming proposals. Thanks to it, most diff --git a/solidity/test/bridge/WalletProposalValidator.test.ts b/solidity/test/bridge/WalletProposalValidator.test.ts index e00960d78..fe7f518a2 100644 --- a/solidity/test/bridge/WalletProposalValidator.test.ts +++ b/solidity/test/bridge/WalletProposalValidator.test.ts @@ -1897,10 +1897,15 @@ describe("WalletProposalValidator", () => { describe("validateMovingFundsProposal", () => { const walletPubKeyHash = "0x7ac2d9378a1c47e589dfb8095ca95ed2140d2726" - const ecdsaWalletID = - "0x4ad6b3ccbca81645865d8d0d575797a15528e98ced22f29a6f906d3259569863" + const targetWallets = [ + "0x84a70187011e156686788e0a2bc50944a4721e83", + "0xf64a45c07e3778b8ce58cb0058477c821c543aad", + "0xcaea95433d9bfa80bb8dc8819a48e2a9aa96147c", + ] + // Hash calculated from the above target wallets. + const targetWalletsHash = + "0x16311d424d513a1743fbc9c0e4fea5b70eddefd15f54613503e5cdfab24f8877" const movingFundsTxMaxTotalFee = 20000 - const walletMaxBtcTransfer = 30000000 before(async () => { await createSnapshot() @@ -1918,12 +1923,10 @@ describe("WalletProposalValidator", () => { 0, 0, ]) - bridge.walletParameters.returns([0, 0, 0, 0, 0, walletMaxBtcTransfer, 0]) }) after(async () => { bridge.movingFundsParameters.reset() - bridge.walletParameters.reset() await restoreSnapshot() }) @@ -1958,7 +1961,7 @@ describe("WalletProposalValidator", () => { await createSnapshot() bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ - ecdsaWalletID, + ecdsaWalletID: HashZero, mainUtxoHash: HashZero, pendingRedemptionsValue: 0, createdAt: 0, @@ -1978,15 +1981,11 @@ describe("WalletProposalValidator", () => { it("should revert", async () => { await expect( - // Only walletPubKeyHash argument is relevant in this scenario. - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [], - }, - NO_MAIN_UTXO - ) + walletProposalValidator.validateMovingFundsProposal({ + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }) ).to.be.revertedWith("Source wallet is not in MovingFunds state") }) }) @@ -1994,19 +1993,20 @@ describe("WalletProposalValidator", () => { }) context("when wallet's state is MovingFunds", () => { - context("when the wallet has pending redemptions", () => { + context("when moving funds commitment has not been submitted", () => { before(async () => { await createSnapshot() bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ - ecdsaWalletID, + ecdsaWalletID: HashZero, mainUtxoHash: HashZero, - pendingRedemptionsValue: 10000, // Make the value positive. + pendingRedemptionsValue: 0, createdAt: 0, movingFundsRequestedAt: 0, closingStartedAt: 0, pendingMovedFundsSweepRequestsCount: 0, state: walletState.MovingFunds, + // Indicate the commitment has not been submitted. movingFundsTargetWalletsCommitmentHash: HashZero, }) }) @@ -2019,451 +2019,115 @@ describe("WalletProposalValidator", () => { it("should revert", async () => { await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [], - }, - NO_MAIN_UTXO - ) - ).to.be.revertedWith("Source wallet has pending redemptions") + walletProposalValidator.validateMovingFundsProposal({ + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }) + ).to.be.revertedWith("Target wallets commitment is not submitted") }) }) - context("when the wallet does not have pending redemptions", () => { - context( - "when the wallet has pending moved funds sweep requests", - () => { - before(async () => { - await createSnapshot() - - bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ - ecdsaWalletID, - mainUtxoHash: HashZero, - pendingRedemptionsValue: 0, - createdAt: 0, - movingFundsRequestedAt: 0, - closingStartedAt: 0, - pendingMovedFundsSweepRequestsCount: 1, // Make the value positive. - state: walletState.MovingFunds, - movingFundsTargetWalletsCommitmentHash: HashZero, - }) - }) - - after(async () => { - bridge.wallets.reset() + context("when moving funds commitment has been submitted", () => { + context("when commitment hash does not match target wallets", () => { + before(async () => { + await createSnapshot() - await restoreSnapshot() + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID: HashZero, + mainUtxoHash: HashZero, + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.MovingFunds, + // Set a hash that does not match the target wallets. + movingFundsTargetWalletsCommitmentHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", }) + }) - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [], - }, - NO_MAIN_UTXO - ) - ).to.be.revertedWith( - "Source wallet has pending moved funds sweep requests" - ) - }) - } - ) + after(async () => { + bridge.wallets.reset() - context( - "when the wallet does not have pending moved funds sweep requests", - () => { - context("when the provided main UTXO is invalid", () => { - before(async () => { - await createSnapshot() + await restoreSnapshot() + }) - bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ - ecdsaWalletID, - mainUtxoHash: - // Use any non-zero hash to indicate the wallet has a main UTXO. - "0x1111111111111111111111111111111111111111111111111111111111111111", - pendingRedemptionsValue: 0, - createdAt: 0, - movingFundsRequestedAt: 0, - closingStartedAt: 0, - pendingMovedFundsSweepRequestsCount: 0, - state: walletState.MovingFunds, - movingFundsTargetWalletsCommitmentHash: HashZero, - }) + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal({ + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets, }) + ).to.be.revertedWith( + "Target wallets do not match target wallets commitment hash" + ) + }) + }) - after(async () => { - bridge.wallets.reset() - - await restoreSnapshot() - }) + context("when commitment hash matches target wallets", () => { + before(async () => { + await createSnapshot() - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [], - }, - { - txHash: - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - txOutputIndex: 0, - txOutputValue: 1000, - } - ) - ).to.be.revertedWith("Invalid wallet main UTXO data") - }) + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID: HashZero, + mainUtxoHash: HashZero, + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.MovingFunds, + // Set the correct hash. + movingFundsTargetWalletsCommitmentHash: targetWalletsHash, }) + }) - context("when the provided main UTXO is valid", () => { - context("when the wallet's balance is zero", () => { - before(async () => { - await createSnapshot() - - bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ - ecdsaWalletID, - // Use zero hash so that the wallet's main is considered not - // set. - mainUtxoHash: HashZero, - pendingRedemptionsValue: 0, - createdAt: 0, - movingFundsRequestedAt: 0, - closingStartedAt: 0, - pendingMovedFundsSweepRequestsCount: 0, - state: walletState.MovingFunds, - movingFundsTargetWalletsCommitmentHash: HashZero, - }) - }) - - after(async () => { - bridge.wallets.reset() - - await restoreSnapshot() - }) - - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [], - }, - NO_MAIN_UTXO - ) - ).to.be.revertedWith("Source wallet BTC balance is zero") - }) - }) - - context("when the wallet's balance is positive", () => { - const targetWallets = [ - "0x1111111111111111111111111111111111111111", - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - "0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", - ] - - before(async () => { - await createSnapshot() - - // Set the source wallet. - bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ - ecdsaWalletID, - mainUtxoHash: - "0x2cd9812b371968dc7e244c6757778d436cda256db583909a1758cac47c5d3df9", - pendingRedemptionsValue: 0, - createdAt: 0, - movingFundsRequestedAt: 0, - closingStartedAt: 0, - pendingMovedFundsSweepRequestsCount: 0, - state: walletState.MovingFunds, - movingFundsTargetWalletsCommitmentHash: HashZero, - }) - - // Set target wallets. - targetWallets.forEach((targetWallet) => { - bridge.wallets.whenCalledWith(targetWallet).returns({ - ecdsaWalletID, - mainUtxoHash: HashZero, - pendingRedemptionsValue: 0, - createdAt: 0, - movingFundsRequestedAt: 0, - closingStartedAt: 0, - pendingMovedFundsSweepRequestsCount: 0, - state: walletState.Live, - movingFundsTargetWalletsCommitmentHash: HashZero, - }) - }) - }) + after(async () => { + bridge.wallets.reset() - after(async () => { - bridge.wallets.reset() + await restoreSnapshot() + }) - await restoreSnapshot() + context("when transaction fee is zero", () => { + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal({ + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets, }) + ).to.be.revertedWith("Proposed transaction fee cannot be zero") + }) + }) - context("when there are no target wallets available", () => { - before(async () => { - await createSnapshot() - - bridge.liveWalletsCount.returns(0) - }) - - after(async () => { - bridge.liveWalletsCount.reset() - - await restoreSnapshot() - }) - - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [], - }, - { - txHash: - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - txOutputIndex: 0, - txOutputValue: 100000000, - } - ) - ).to.be.revertedWith("No target wallets available") - }) + context("when transaction fee is too high", () => { + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal({ + walletPubKeyHash, + movingFundsTxFee: movingFundsTxMaxTotalFee + 1, + targetWallets, }) + ).to.be.revertedWith("Proposed transaction fee is too high") + }) + }) - context("when there are target wallets available", () => { - before(async () => { - await createSnapshot() - - bridge.liveWalletsCount.returns(3) - }) - - after(async () => { - bridge.liveWalletsCount.reset() - - await restoreSnapshot() - }) - - context( - "when the number of target wallets is other than expected", - () => { - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [ - // Provide one target wallet instead of three. - targetWallets[0], - ], - }, - { - txHash: - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - txOutputIndex: 0, - txOutputValue: 100000000, - } - ) - ).to.be.revertedWith( - "Submitted target wallets count is other than expected" - ) - }) - } - ) - - context( - "when the number of target wallets is expected", - () => { - context( - "when the source wallet is used as a target wallet", - () => { - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [ - "0x1111111111111111111111111111111111111111", - walletPubKeyHash, // Use source wallet. - "0x3333333333333333333333333333333333333333", - ], - }, - { - txHash: - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - txOutputIndex: 0, - txOutputValue: 100000000, - } - ) - ).to.be.revertedWith( - "Target wallet is equal to source wallet" - ) - }) - } - ) - - context( - "when the source wallet is not used as a target wallet", - () => { - context( - "when the order of target wallets is incorrect", - () => { - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [ - // Change order. - targetWallets[0], - targetWallets[2], - targetWallets[1], - ], - }, - { - txHash: - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - txOutputIndex: 0, - txOutputValue: 100000000, - } - ) - ).to.be.revertedWith( - "Target wallet order is incorrect" - ) - }) - } - ) - - context( - "when the order of target wallets is correct", - () => { - context( - "when one of the target wallets is not Live", - () => { - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [ - targetWallets[0], - targetWallets[1], - // Use unknown wallet, so that its state is not Unknown. - "0xcccccccccccccccccccccccccccccccccccccccc", - ], - }, - { - txHash: - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - txOutputIndex: 0, - txOutputValue: 100000000, - } - ) - ).to.be.revertedWith( - "Target wallet is not in Live state" - ) - }) - } - ) - - context( - "when all the target wallets are Live", - () => { - context( - "when the provided transaction fee is incorrect", - () => { - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets, - }, - { - txHash: - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - txOutputIndex: 0, - txOutputValue: 100000000, - } - ) - ).to.be.revertedWith( - "Proposed transaction fee cannot be zero" - ) - }) - - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: - movingFundsTxMaxTotalFee + 1, - targetWallets, - }, - { - txHash: - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - txOutputIndex: 0, - txOutputValue: 100000000, - } - ) - ).to.be.revertedWith( - "Proposed transaction fee is too high" - ) - }) - } - ) - - context( - "when the provided transaction fee is correct", - () => { - it("should pass validation", async () => { - const result = - await walletProposalValidator.validateMovingFundsProposal( - { - walletPubKeyHash, - movingFundsTxFee: - movingFundsTxMaxTotalFee, - targetWallets, - }, - { - txHash: - "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - txOutputIndex: 0, - txOutputValue: 100000000, - } - ) - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - expect(result).to.be.true - }) - } - ) - } - ) - } - ) - } - ) - } - ) + context("when transaction fee is valid", () => { + it("should pass validation", async () => { + const result = + await walletProposalValidator.validateMovingFundsProposal({ + walletPubKeyHash, + movingFundsTxFee: movingFundsTxMaxTotalFee, + targetWallets, }) - }) + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect(result).to.be.true }) - } - ) + }) + }) }) }) })