From db47dbb09b123424f39a5d6c3f2c75a635c04d57 Mon Sep 17 00:00:00 2001 From: Tomasz Slabon Date: Thu, 21 Dec 2023 18:52:56 +0100 Subject: [PATCH] Added source wallet BTC balance check --- .../bridge/WalletProposalValidator.sol | 76 ++++- .../bridge/WalletProposalValidator.test.ts | 290 ++++++++++++++---- 2 files changed, 296 insertions(+), 70 deletions(-) diff --git a/solidity/contracts/bridge/WalletProposalValidator.sol b/solidity/contracts/bridge/WalletProposalValidator.sol index 5163f3186..39f7ef5fa 100644 --- a/solidity/contracts/bridge/WalletProposalValidator.sol +++ b/solidity/contracts/bridge/WalletProposalValidator.sol @@ -603,6 +603,7 @@ 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 Notice that this function is meant to be invoked after the moving /// funds commitment has already been submitted. This function skips @@ -613,15 +614,17 @@ contract WalletProposalValidator { /// - The target wallets commitment must be submitted, /// - The target wallets commitment hash must match the target wallets /// from the proposal, + /// - The source wallet BTC balance must be positive, + /// - The source wallet BTC balance must be equal to or greater than + /// `movingFundsDustThreshold`, /// - 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) - { + function validateMovingFundsProposal( + MovingFundsProposal calldata proposal, + BitcoinTx.UTXO calldata walletMainUtxo + ) external view returns (bool) { Wallets.Wallet memory sourceWallet = bridge.wallets( proposal.walletPubKeyHash ); @@ -645,10 +648,37 @@ contract WalletProposalValidator { "Target wallets do not match target wallets commitment hash" ); - // Make sure the proposed fee is valid. - (uint64 movingFundsTxMaxTotalFee, , , , , , , , , , ) = bridge - .movingFundsParameters(); + ( + uint64 movingFundsTxMaxTotalFee, + uint64 movingFundsDustThreshold, + , + , + , + , + , + , + , + , + + ) = bridge.movingFundsParameters(); + + // Make sure the source wallet balance is correct. + uint64 sourceWalletBtcBalance = getWalletBtcBalance( + sourceWallet.mainUtxoHash, + walletMainUtxo + ); + require( + sourceWalletBtcBalance > 0, + "Source wallet BTC balance is zero" + ); + + require( + sourceWalletBtcBalance >= movingFundsDustThreshold, + "Source wallet BTC balance is below the moving funds dust threshold" + ); + + // Make sure the proposed fee is valid. require( proposal.movingFundsTxFee > 0, "Proposed transaction fee cannot be zero" @@ -662,6 +692,36 @@ 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 fe7f518a2..afee20bda 100644 --- a/solidity/test/bridge/WalletProposalValidator.test.ts +++ b/solidity/test/bridge/WalletProposalValidator.test.ts @@ -1896,6 +1896,8 @@ describe("WalletProposalValidator", () => { }) describe("validateMovingFundsProposal", () => { + const movingFundsTxMaxTotalFee = 20000 + const movingFundsDustThreshold = 5000 const walletPubKeyHash = "0x7ac2d9378a1c47e589dfb8095ca95ed2140d2726" const targetWallets = [ "0x84a70187011e156686788e0a2bc50944a4721e83", @@ -1905,14 +1907,22 @@ describe("WalletProposalValidator", () => { // Hash calculated from the above target wallets. const targetWalletsHash = "0x16311d424d513a1743fbc9c0e4fea5b70eddefd15f54613503e5cdfab24f8877" - const movingFundsTxMaxTotalFee = 20000 + const walletMainUtxo = { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: movingFundsDustThreshold, + } + // Hash calculated from the above main UTXO. + const walletMainUtxoHash = + "0x4cfb92c890e30ff736656e519167cbfcacb408c730fd21bec415359b45769d20" before(async () => { await createSnapshot() bridge.movingFundsParameters.returns([ movingFundsTxMaxTotalFee, - 0, + movingFundsDustThreshold, 0, 0, 0, @@ -1981,11 +1991,14 @@ describe("WalletProposalValidator", () => { it("should revert", async () => { await expect( - walletProposalValidator.validateMovingFundsProposal({ - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [], - }) + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }, + NO_MAIN_UTXO + ) ).to.be.revertedWith("Source wallet is not in MovingFunds state") }) }) @@ -2019,11 +2032,14 @@ describe("WalletProposalValidator", () => { it("should revert", async () => { await expect( - walletProposalValidator.validateMovingFundsProposal({ - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets: [], - }) + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets: [], + }, + NO_MAIN_UTXO + ) ).to.be.revertedWith("Target wallets commitment is not submitted") }) }) @@ -2056,11 +2072,14 @@ describe("WalletProposalValidator", () => { it("should revert", async () => { await expect( - walletProposalValidator.validateMovingFundsProposal({ - walletPubKeyHash, - movingFundsTxFee: 0, - targetWallets, - }) + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets, + }, + NO_MAIN_UTXO + ) ).to.be.revertedWith( "Target wallets do not match target wallets commitment hash" ) @@ -2068,64 +2087,211 @@ describe("WalletProposalValidator", () => { }) context("when commitment hash matches target wallets", () => { - before(async () => { - await createSnapshot() - - 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 passed main UTXO is incorrect", () => { + before(async () => { + await createSnapshot() + + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID: HashZero, + 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: targetWalletsHash, + }) }) - }) - 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") + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets, + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: 1000, + } + ) + ).to.be.revertedWith("Invalid wallet main UTXO data") }) }) - context("when transaction fee is too high", () => { - it("should revert", async () => { - await expect( - walletProposalValidator.validateMovingFundsProposal({ - walletPubKeyHash, - movingFundsTxFee: movingFundsTxMaxTotalFee + 1, - targetWallets, + context("when the passed main UTXO is correct", () => { + context("when source wallet BTC balance is zero", () => { + before(async () => { + await createSnapshot() + + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID: HashZero, + // 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: targetWalletsHash, }) - ).to.be.revertedWith("Proposed transaction fee is too high") + }) + + 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 transaction fee is valid", () => { - it("should pass validation", async () => { - const result = - await walletProposalValidator.validateMovingFundsProposal({ - walletPubKeyHash, - movingFundsTxFee: movingFundsTxMaxTotalFee, - targetWallets, + context( + "when source wallet BTC balance is below dust threshold", + () => { + before(async () => { + await createSnapshot() + + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID: HashZero, + mainUtxoHash: + "0x757a5ca2a1e5fff2f2a51c073cb88c097603285fcfa52cb58473704647fa7edb", + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.MovingFunds, + movingFundsTargetWalletsCommitmentHash: targetWalletsHash, + }) }) - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - expect(result).to.be.true - }) + + after(async () => { + bridge.wallets.reset() + + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets, + }, + { + txHash: + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + txOutputIndex: 0, + txOutputValue: movingFundsDustThreshold - 1, + } + ) + ).to.be.revertedWith( + "Source wallet BTC balance is below the moving funds dust threshold" + ) + }) + } + ) + + context( + "when source wallet BTC balance is equal to or greater that dust threshold", + () => { + before(async () => { + await createSnapshot() + bridge.wallets.whenCalledWith(walletPubKeyHash).returns({ + ecdsaWalletID: HashZero, + mainUtxoHash: walletMainUtxoHash, + pendingRedemptionsValue: 0, + createdAt: 0, + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.MovingFunds, + movingFundsTargetWalletsCommitmentHash: targetWalletsHash, + }) + }) + + after(async () => { + bridge.wallets.reset() + + await restoreSnapshot() + }) + + context("when transaction fee is zero", () => { + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: 0, + targetWallets, + }, + walletMainUtxo + ) + ).to.be.revertedWith( + "Proposed transaction fee cannot be zero" + ) + }) + }) + + context("when transaction fee is too high", () => { + it("should revert", async () => { + await expect( + walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: movingFundsTxMaxTotalFee + 1, + targetWallets, + }, + walletMainUtxo + ) + ).to.be.revertedWith("Proposed transaction fee is too high") + }) + }) + + context("when transaction fee is valid", () => { + it("should pass validation", async () => { + const result = + await walletProposalValidator.validateMovingFundsProposal( + { + walletPubKeyHash, + movingFundsTxFee: movingFundsTxMaxTotalFee, + targetWallets, + }, + walletMainUtxo + ) + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect(result).to.be.true + }) + }) + } + ) }) }) })