From 9936af53b5e577673f8eac73b9f549061f825269 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 20 Feb 2024 17:16:56 +0100 Subject: [PATCH] Add watchtower check upon redemption request From now on, the `Bridge` consults the `RedemptionWatchtower` while a new redemption request arises. The redemption request is allowed only if: - The balance owner is not banned in the watchtower - The redeemer is not banned in the watchtower - The redemption key was not objected to in the past --- solidity/contracts/bridge/Redemption.sol | 34 +- .../contracts/bridge/RedemptionWatchtower.sol | 41 + .../test/bridge/Bridge.Redemption.test.ts | 1176 +++++++++-------- .../test/bridge/RedemptionWatchtower.test.ts | 140 ++ 4 files changed, 866 insertions(+), 525 deletions(-) diff --git a/solidity/contracts/bridge/Redemption.sol b/solidity/contracts/bridge/Redemption.sol index bcba9ff91..4969848da 100644 --- a/solidity/contracts/bridge/Redemption.sol +++ b/solidity/contracts/bridge/Redemption.sol @@ -24,6 +24,27 @@ import "./Wallets.sol"; import "../bank/Bank.sol"; +/// @notice Interface of the RedemptionWatchtower. +interface IRedemptionWatchtower { + /// @notice Determines whether a redemption request is considered safe. + /// @param walletPubKeyHash 20-byte public key hash of the wallet that + /// is meant to handle the redemption request. + /// @param redeemerOutputScript The redeemer's length-prefixed output + /// script (P2PKH, P2WPKH, P2SH or P2WSH) that is meant to + /// receive the redeemed amount. + /// @param balanceOwner The address of the Bank balance owner whose balance + /// is getting redeemed. + /// @param redeemer The address that requested the redemption. + /// @return True if the redemption request is safe, false otherwise. + /// Specific safety criteria depend on the implementation. + function isSafeRedemption( + bytes20 walletPubKeyHash, + bytes calldata redeemerOutputScript, + address balanceOwner, + address redeemer + ) external view returns (bool); +} + /// @notice Aggregates functions common to the redemption transaction proof /// validation and to the moving funds transaction proof validation. library OutboundTx { @@ -402,7 +423,18 @@ library Redemption { bytes memory redeemerOutputScript, uint64 amount ) internal { - // TODO: Validate the request against the RedemptionWatchtower. + if (self.redemptionWatchtower != address(0)) { + require( + IRedemptionWatchtower(self.redemptionWatchtower) + .isSafeRedemption( + walletPubKeyHash, + redeemerOutputScript, + balanceOwner, + redeemer + ), + "Redemption request rejected by the watchtower" + ); + } Wallets.Wallet storage wallet = self.registeredWallets[ walletPubKeyHash diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index 15adab777..f464fcfae 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -425,4 +425,45 @@ contract RedemptionWatchtower is OwnableUpgradeable { _levelTwoDelay ); } + + /// @notice Determines whether a redemption request is considered safe. + /// @param walletPubKeyHash 20-byte public key hash of the wallet that + /// is meant to handle the redemption request. + /// @param redeemerOutputScript The redeemer's length-prefixed output + /// script (P2PKH, P2WPKH, P2SH or P2WSH) that is meant to + /// receive the redeemed amount. + /// @param balanceOwner The address of the Bank balance owner whose balance + /// is getting redeemed. + /// @param redeemer The address that requested the redemption. + /// @return True if the redemption request is safe, false otherwise. + /// The redemption is considered safe when: + /// - The balance owner is not banned, + /// - The redeemer is not banned, + /// - There are no objections against past redemptions from the + /// given wallet to the given redeemer output script. + function isSafeRedemption( + bytes20 walletPubKeyHash, + bytes calldata redeemerOutputScript, + address balanceOwner, + address redeemer + ) external view returns (bool) { + if (isBanned[balanceOwner]) { + return false; + } + + if (isBanned[redeemer]) { + return false; + } + + uint256 redemptionKey = Redemption.getRedemptionKey( + walletPubKeyHash, + redeemerOutputScript + ); + + if (vetoProposals[redemptionKey].objectionsCount > 0) { + return false; + } + + return true; + } } diff --git a/solidity/test/bridge/Bridge.Redemption.test.ts b/solidity/test/bridge/Bridge.Redemption.test.ts index bdf154304..a8893404f 100644 --- a/solidity/test/bridge/Bridge.Redemption.test.ts +++ b/solidity/test/bridge/Bridge.Redemption.test.ts @@ -6,37 +6,38 @@ import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers" import chai, { expect } from "chai" import { BigNumber, BigNumberish, Contract, ContractTransaction } from "ethers" import { BytesLike } from "@ethersproject/bytes" -import { smock } from "@defi-wonderland/smock" import type { FakeContract } from "@defi-wonderland/smock" +import { smock } from "@defi-wonderland/smock" import { Deployment } from "hardhat-deploy/types" import type { Bank, BankStub, Bridge, + BridgeGovernance, BridgeStub, - IWalletRegistry, + IRedemptionWatchtower, IRelay, - BridgeGovernance, + IWalletRegistry, } from "../../typechain" import { NO_MAIN_UTXO } from "../data/deposit-sweep" import { MultiplePendingRequestedRedemptions, + MultiplePendingRequestedRedemptionsWithMultipleInputs, + MultiplePendingRequestedRedemptionsWithMultipleP2WPKHChanges, + MultiplePendingRequestedRedemptionsWithNonRequestedRedemption, + MultiplePendingRequestedRedemptionsWithP2SHChange, MultiplePendingRequestedRedemptionsWithP2WPKHChange, + MultiplePendingRequestedRedemptionsWithP2WPKHChangeZeroValue, + MultiplePendingRequestedRedemptionsWithProvablyUnspendable, RedemptionBalanceChange, RedemptionTestData, + SingleNonRequestedRedemption, SingleP2PKHChange, SingleP2SHChange, SingleP2WPKHChange, SingleP2WPKHChangeZeroValue, - SingleNonRequestedRedemption, SinglePendingRequestedRedemption, SingleProvablyUnspendable, - MultiplePendingRequestedRedemptionsWithP2SHChange, - MultiplePendingRequestedRedemptionsWithMultipleP2WPKHChanges, - MultiplePendingRequestedRedemptionsWithP2WPKHChangeZeroValue, - MultiplePendingRequestedRedemptionsWithNonRequestedRedemption, - MultiplePendingRequestedRedemptionsWithProvablyUnspendable, - MultiplePendingRequestedRedemptionsWithMultipleInputs, } from "../data/redemption" import { constants, walletState } from "../fixtures" import bridgeFixture from "../fixtures/bridge" @@ -124,401 +125,447 @@ describe("Bridge - Redemption", () => { describe("requestRedemption", () => { const walletPubKeyHash = "0x8db50eb52063ea9d98b3eac91489a90f738986f6" - context("when wallet state is Live", () => { - before(async () => { - await createSnapshot() - - // Simulate the wallet is an Live one and is known to the system. - await bridge.setWallet(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 there is a main UTXO for the given wallet", () => { - // Prepare a dumb main UTXO with 10M satoshi as value. This will - // be the wallet BTC balance. - const mainUtxo = { - txHash: - "0x3835ecdee2daa83c9a19b5012104ace55ecab197b5e16489c26d372e475f5d2a", - txOutputIndex: 0, - txOutputValue: 10000000, - } - + context("when redemption watchtower is not set", () => { + context("when wallet state is Live", () => { before(async () => { await createSnapshot() - // Simulate the prepared main UTXO belongs to the wallet. - await bridge.setWalletMainUtxo(walletPubKeyHash, mainUtxo) + // Simulate the wallet is an Live one and is known to the system. + await bridge.setWallet(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 main UTXO data are valid", () => { - context("when redeemer output script is standard type", () => { - // Arbitrary standard output scripts. - const redeemerOutputScriptP2WPKH = - "0x160014f4eedc8f40d4b8e30771f792b065ebec0abaddef" - const redeemerOutputScriptP2WSH = - "0x220020ef0b4d985752aa5ef6243e4c6f6bebc2a007e7d671ef27d4b1d0db8dcc93bc1c" - const redeemerOutputScriptP2PKH = - "0x1976a914f4eedc8f40d4b8e30771f792b065ebec0abaddef88ac" - const redeemerOutputScriptP2SH = - "0x17a914f4eedc8f40d4b8e30771f792b065ebec0abaddef87" - - context( - "when redeemer output script does not point to the wallet public key hash", - () => { - context("when amount is not below the dust threshold", () => { - // Requested amount is 1901000 satoshi. - const requestedAmount = BigNumber.from(1901000) - // Treasury fee is `requestedAmount / redemptionTreasuryFeeDivisor` - // where the divisor is `2000` initially. So, we - // have 1901000 / 2000 = 950.5 though Solidity - // loses the decimal part. - const treasuryFee = 950 - - context( - "when there is no pending request for the given redemption key", - () => { - context("when wallet has sufficient funds", () => { - context( - "when redeemer made a sufficient allowance in Bank", - () => { - let redeemer: SignerWithAddress + context("when there is a main UTXO for the given wallet", () => { + // Prepare a dumb main UTXO with 10M satoshi as value. This will + // be the wallet BTC balance. + const mainUtxo = { + txHash: + "0x3835ecdee2daa83c9a19b5012104ace55ecab197b5e16489c26d372e475f5d2a", + txOutputIndex: 0, + txOutputValue: 10000000, + } - before(async () => { - await createSnapshot() + before(async () => { + await createSnapshot() - // Use an arbitrary ETH account as redeemer. - redeemer = thirdParty + // Simulate the prepared main UTXO belongs to the wallet. + await bridge.setWalletMainUtxo(walletPubKeyHash, mainUtxo) + }) - await makeRedemptionAllowance( - redeemer, - requestedAmount - ) - }) + after(async () => { + await restoreSnapshot() + }) - after(async () => { - await restoreSnapshot() - }) + context("when main UTXO data are valid", () => { + context("when redeemer output script is standard type", () => { + // Arbitrary standard output scripts. + const redeemerOutputScriptP2WPKH = + "0x160014f4eedc8f40d4b8e30771f792b065ebec0abaddef" + const redeemerOutputScriptP2WSH = + "0x220020ef0b4d985752aa5ef6243e4c6f6bebc2a007e7d671ef27d4b1d0db8dcc93bc1c" + const redeemerOutputScriptP2PKH = + "0x1976a914f4eedc8f40d4b8e30771f792b065ebec0abaddef88ac" + const redeemerOutputScriptP2SH = + "0x17a914f4eedc8f40d4b8e30771f792b065ebec0abaddef87" - context( - "when redeemer output script is P2WPKH", - () => { - const redeemerOutputScript = - redeemerOutputScriptP2WPKH + context( + "when redeemer output script does not point to the wallet public key hash", + () => { + context("when amount is not below the dust threshold", () => { + // Requested amount is 1901000 satoshi. + const requestedAmount = BigNumber.from(1901000) + // Treasury fee is `requestedAmount / redemptionTreasuryFeeDivisor` + // where the divisor is `2000` initially. So, we + // have 1901000 / 2000 = 950.5 though Solidity + // loses the decimal part. + const treasuryFee = 950 + + context( + "when there is no pending request for the given redemption key", + () => { + context("when wallet has sufficient funds", () => { + context( + "when redeemer made a sufficient allowance in Bank", + () => { + let redeemer: SignerWithAddress + + before(async () => { + await createSnapshot() + + // Use an arbitrary ETH account as redeemer. + redeemer = thirdParty + + await makeRedemptionAllowance( + redeemer, + requestedAmount + ) + }) - let initialBridgeBalance: BigNumber - let initialRedeemerBalance: BigNumber - let initialWalletPendingRedemptionValue: BigNumber - let tx: ContractTransaction + after(async () => { + await restoreSnapshot() + }) - let redemptionTxMaxFee: BigNumber + context( + "when redeemer output script is P2WPKH", + () => { + const redeemerOutputScript = + redeemerOutputScriptP2WPKH - before(async () => { - await createSnapshot() + let initialBridgeBalance: BigNumber + let initialRedeemerBalance: BigNumber + let initialWalletPendingRedemptionValue: BigNumber + let tx: ContractTransaction - redemptionTxMaxFee = ( - await bridge.redemptionParameters() - ).redemptionTxMaxFee + let redemptionTxMaxFee: BigNumber - // Capture initial balance of Bridge and - // redeemer. - initialBridgeBalance = await bank.balanceOf( - bridge.address - ) - initialRedeemerBalance = await bank.balanceOf( - redeemer.address - ) + before(async () => { + await createSnapshot() - // Capture the initial pending redemptions value - // for the given wallet. - initialWalletPendingRedemptionValue = ( - await bridge.wallets(walletPubKeyHash) - ).pendingRedemptionsValue + redemptionTxMaxFee = ( + await bridge.redemptionParameters() + ).redemptionTxMaxFee - // Perform the redemption request. - tx = await bridge - .connect(redeemer) - .requestRedemption( - walletPubKeyHash, - mainUtxo, - redeemerOutputScript, - requestedAmount + // Capture initial balance of Bridge and + // redeemer. + initialBridgeBalance = await bank.balanceOf( + bridge.address ) - }) + initialRedeemerBalance = + await bank.balanceOf(redeemer.address) - after(async () => { - await restoreSnapshot() - }) + // Capture the initial pending redemptions value + // for the given wallet. + initialWalletPendingRedemptionValue = ( + await bridge.wallets(walletPubKeyHash) + ).pendingRedemptionsValue - it("should increase the wallet's pending redemptions value", async () => { - const walletPendingRedemptionValue = ( - await bridge.wallets(walletPubKeyHash) - ).pendingRedemptionsValue + // Perform the redemption request. + tx = await bridge + .connect(redeemer) + .requestRedemption( + walletPubKeyHash, + mainUtxo, + redeemerOutputScript, + requestedAmount + ) + }) - expect( - walletPendingRedemptionValue.sub( - initialWalletPendingRedemptionValue - ) - ).to.be.equal( - requestedAmount.sub(treasuryFee) - ) - }) + after(async () => { + await restoreSnapshot() + }) - it("should store the redemption request", async () => { - const redemptionKey = buildRedemptionKey( - walletPubKeyHash, - redeemerOutputScript - ) + it("should increase the wallet's pending redemptions value", async () => { + const walletPendingRedemptionValue = ( + await bridge.wallets(walletPubKeyHash) + ).pendingRedemptionsValue - const redemptionRequest = - await bridge.pendingRedemptions( - redemptionKey + expect( + walletPendingRedemptionValue.sub( + initialWalletPendingRedemptionValue + ) + ).to.be.equal( + requestedAmount.sub(treasuryFee) ) + }) - expect( - redemptionRequest.redeemer - ).to.be.equal(redeemer.address) - expect( - redemptionRequest.requestedAmount - ).to.be.equal(requestedAmount) - expect( - redemptionRequest.treasuryFee - ).to.be.equal(treasuryFee) - expect( - redemptionRequest.txMaxFee - ).to.be.equal(redemptionTxMaxFee) - expect( - redemptionRequest.requestedAt - ).to.be.equal(await lastBlockTime()) - }) - - it("should emit RedemptionRequested event", async () => { - await expect(tx) - .to.emit(bridge, "RedemptionRequested") - .withArgs( + it("should store the redemption request", async () => { + const redemptionKey = buildRedemptionKey( walletPubKeyHash, - redeemerOutputScript, - redeemer.address, - requestedAmount, - treasuryFee, - redemptionTxMaxFee + redeemerOutputScript ) - }) - - it("should take the right balance from Bank", async () => { - const bridgeBalance = await bank.balanceOf( - bridge.address - ) - expect( - bridgeBalance.sub(initialBridgeBalance) - ).to.equal(requestedAmount) - const redeemerBalance = await bank.balanceOf( - redeemer.address - ) - expect( - redeemerBalance.sub(initialRedeemerBalance) - ).to.equal(requestedAmount.mul(-1)) - }) - } - ) + const redemptionRequest = + await bridge.pendingRedemptions( + redemptionKey + ) - context( - "when redeemer output script is P2WSH", - () => { - before(async () => { - await createSnapshot() - }) - - after(async () => { - await restoreSnapshot() - }) - - // Do not repeat all checks made in the - // "when redeemer output script is P2WPKH" - // scenario but just assert the call succeeds - // for an P2WSH output script. - it("should succeed", async () => { - await expect( - bridge - .connect(redeemer) - .requestRedemption( + expect( + redemptionRequest.redeemer + ).to.be.equal(redeemer.address) + expect( + redemptionRequest.requestedAmount + ).to.be.equal(requestedAmount) + expect( + redemptionRequest.treasuryFee + ).to.be.equal(treasuryFee) + expect( + redemptionRequest.txMaxFee + ).to.be.equal(redemptionTxMaxFee) + expect( + redemptionRequest.requestedAt + ).to.be.equal(await lastBlockTime()) + }) + + it("should emit RedemptionRequested event", async () => { + await expect(tx) + .to.emit(bridge, "RedemptionRequested") + .withArgs( walletPubKeyHash, - mainUtxo, - redeemerOutputScriptP2WSH, - requestedAmount + redeemerOutputScript, + redeemer.address, + requestedAmount, + treasuryFee, + redemptionTxMaxFee ) - ).to.not.be.reverted - }) - } - ) + }) - context( - "when redeemer output script is P2PKH", - () => { - before(async () => { - await createSnapshot() - }) - - after(async () => { - await restoreSnapshot() - }) - - // Do not repeat all checks made in the - // "when redeemer output script is P2WPKH" - // scenario but just assert the call succeeds - // for an P2PKH output script. - it("should succeed", async () => { - await expect( - bridge - .connect(redeemer) - .requestRedemption( - walletPubKeyHash, - mainUtxo, - redeemerOutputScriptP2PKH, - requestedAmount + it("should take the right balance from Bank", async () => { + const bridgeBalance = await bank.balanceOf( + bridge.address + ) + expect( + bridgeBalance.sub(initialBridgeBalance) + ).to.equal(requestedAmount) + + const redeemerBalance = + await bank.balanceOf(redeemer.address) + expect( + redeemerBalance.sub( + initialRedeemerBalance ) - ).to.not.be.reverted - }) - } - ) + ).to.equal(requestedAmount.mul(-1)) + }) + } + ) + + context( + "when redeemer output script is P2WSH", + () => { + before(async () => { + await createSnapshot() + }) + + after(async () => { + await restoreSnapshot() + }) + + // Do not repeat all checks made in the + // "when redeemer output script is P2WPKH" + // scenario but just assert the call succeeds + // for an P2WSH output script. + it("should succeed", async () => { + await expect( + bridge + .connect(redeemer) + .requestRedemption( + walletPubKeyHash, + mainUtxo, + redeemerOutputScriptP2WSH, + requestedAmount + ) + ).to.not.be.reverted + }) + } + ) + + context( + "when redeemer output script is P2PKH", + () => { + before(async () => { + await createSnapshot() + }) + + after(async () => { + await restoreSnapshot() + }) + + // Do not repeat all checks made in the + // "when redeemer output script is P2WPKH" + // scenario but just assert the call succeeds + // for an P2PKH output script. + it("should succeed", async () => { + await expect( + bridge + .connect(redeemer) + .requestRedemption( + walletPubKeyHash, + mainUtxo, + redeemerOutputScriptP2PKH, + requestedAmount + ) + ).to.not.be.reverted + }) + } + ) + + context( + "when redeemer output script is P2SH", + () => { + before(async () => { + await createSnapshot() + }) + + after(async () => { + await restoreSnapshot() + }) + + // Do not repeat all checks made in the + // "when redeemer output script is P2WPKH" + // scenario but just assert the call succeeds + // for an P2SH output script. + it("should succeed", async () => { + await expect( + bridge + .connect(redeemer) + .requestRedemption( + walletPubKeyHash, + mainUtxo, + redeemerOutputScriptP2SH, + requestedAmount + ) + ).to.not.be.reverted + }) + } + ) + + context( + "when redemption treasury fee is zero", + () => { + const redeemerOutputScript = + redeemerOutputScriptP2WPKH + + before(async () => { + await createSnapshot() + + await bridgeGovernance + .connect(governance) + .beginRedemptionTreasuryFeeDivisorUpdate( + 0 + ) + await helpers.time.increaseTime( + constants.governanceDelay + ) + await bridgeGovernance + .connect(governance) + .finalizeRedemptionTreasuryFeeDivisorUpdate() - context( - "when redeemer output script is P2SH", - () => { - before(async () => { - await createSnapshot() - }) - - after(async () => { - await restoreSnapshot() - }) - - // Do not repeat all checks made in the - // "when redeemer output script is P2WPKH" - // scenario but just assert the call succeeds - // for an P2SH output script. - it("should succeed", async () => { - await expect( - bridge + await bridge .connect(redeemer) .requestRedemption( walletPubKeyHash, mainUtxo, - redeemerOutputScriptP2SH, + redeemerOutputScript, requestedAmount ) - ).to.not.be.reverted - }) - } - ) - - context( - "when redemption treasury fee is zero", - () => { - const redeemerOutputScript = - redeemerOutputScriptP2WPKH + }) + + after(async () => { + await restoreSnapshot() + }) + + // Do not repeat all checks made in the + // "when redeemer output script is P2WPKH" + // scenario but just assert the requested + // amount and treasury fee + it("should store the redemption request with zero fee", async () => { + const redemptionKey = buildRedemptionKey( + walletPubKeyHash, + redeemerOutputScript + ) - before(async () => { - await createSnapshot() + const redemptionRequest = + await bridge.pendingRedemptions( + redemptionKey + ) - await bridgeGovernance - .connect(governance) - .beginRedemptionTreasuryFeeDivisorUpdate(0) - await helpers.time.increaseTime( - constants.governanceDelay - ) - await bridgeGovernance - .connect(governance) - .finalizeRedemptionTreasuryFeeDivisorUpdate() + expect( + redemptionRequest.requestedAmount + ).to.be.equal(requestedAmount) + expect( + redemptionRequest.treasuryFee + ).to.be.equal(0) + }) + } + ) + } + ) - await bridge - .connect(redeemer) + context( + "when redeemer has not made a sufficient allowance in Bank", + () => { + it("should revert", async () => { + await expect( + bridge + .connect(thirdParty) .requestRedemption( walletPubKeyHash, mainUtxo, - redeemerOutputScript, + redeemerOutputScriptP2WPKH, requestedAmount ) - }) - - after(async () => { - await restoreSnapshot() - }) - - // Do not repeat all checks made in the - // "when redeemer output script is P2WPKH" - // scenario but just assert the requested - // amount and treasury fee - it("should store the redemption request with zero fee", async () => { - const redemptionKey = buildRedemptionKey( - walletPubKeyHash, - redeemerOutputScript - ) - - const redemptionRequest = - await bridge.pendingRedemptions( - redemptionKey - ) + ).to.be.revertedWith( + "Transfer amount exceeds allowance" + ) + }) + } + ) + }) - expect( - redemptionRequest.requestedAmount - ).to.be.equal(requestedAmount) - expect( - redemptionRequest.treasuryFee - ).to.be.equal(0) - }) - } + context("when wallet has insufficient funds", () => { + before(async () => { + await createSnapshot() + + // Simulate a situation when the wallet has so many + // pending redemptions that a new request will + // exceed its Bitcoin balance. This is done by making + // a redemption request that will request the entire + // wallet's balance right before the tested request. + await makeRedemptionAllowance( + thirdParty, + mainUtxo.txOutputValue ) - } - ) - - context( - "when redeemer has not made a sufficient allowance in Bank", - () => { - it("should revert", async () => { - await expect( - bridge - .connect(thirdParty) - .requestRedemption( - walletPubKeyHash, - mainUtxo, - redeemerOutputScriptP2WPKH, - requestedAmount - ) - ).to.be.revertedWith( - "Transfer amount exceeds allowance" + await bridge + .connect(thirdParty) + .requestRedemption( + walletPubKeyHash, + mainUtxo, + redeemerOutputScriptP2WPKH, + mainUtxo.txOutputValue ) - }) - } - ) - }) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + bridge + .connect(thirdParty) + .requestRedemption( + walletPubKeyHash, + mainUtxo, + redeemerOutputScriptP2WSH, + requestedAmount + ) + ).to.be.revertedWith("Insufficient wallet funds") + }) + }) + } + ) - context("when wallet has insufficient funds", () => { + context( + "when there is a pending request for the given redemption key", + () => { before(async () => { await createSnapshot() - // Simulate a situation when the wallet has so many - // pending redemptions that a new request will - // exceed its Bitcoin balance. This is done by making - // a redemption request that will request the entire - // wallet's balance right before the tested request. + // Make a request targeting the given wallet and + // redeemer output script. Tested request will use + // the same parameters. await makeRedemptionAllowance( thirdParty, mainUtxo.txOutputValue @@ -544,253 +591,334 @@ describe("Bridge - Redemption", () => { .requestRedemption( walletPubKeyHash, mainUtxo, - redeemerOutputScriptP2WSH, + redeemerOutputScriptP2WPKH, requestedAmount ) - ).to.be.revertedWith("Insufficient wallet funds") + ).to.be.revertedWith( + "There is a pending redemption request from this wallet to the same address" + ) }) - }) - } - ) - - context( - "when there is a pending request for the given redemption key", - () => { - before(async () => { - await createSnapshot() + } + ) + }) - // Make a request targeting the given wallet and - // redeemer output script. Tested request will use - // the same parameters. - await makeRedemptionAllowance( - thirdParty, - mainUtxo.txOutputValue - ) - await bridge + context("when amount is below the dust threshold", () => { + it("should revert", async () => { + // Initial dust threshold set in the tests `fixture` + // for tests is 100000. A value lower by 1 sat should + // trigger the tested condition. + await expect( + bridge .connect(thirdParty) .requestRedemption( walletPubKeyHash, mainUtxo, redeemerOutputScriptP2WPKH, - mainUtxo.txOutputValue + 99999 ) - }) + ).to.be.revertedWith("Redemption amount too small") + }) + }) + } + ) - after(async () => { - await restoreSnapshot() - }) + context( + "when redeemer output script points to the wallet public key hash", + () => { + it("should revert", async () => { + // Wallet public key hash hidden under P2WPKH. + await expect( + bridge + .connect(thirdParty) + .requestRedemption( + walletPubKeyHash, + mainUtxo, + `0x160014${walletPubKeyHash.substring(2)}`, + 100000 + ) + ).to.be.revertedWith( + "Redeemer output script must not point to the wallet PKH" + ) - it("should revert", async () => { - await expect( - bridge - .connect(thirdParty) - .requestRedemption( - walletPubKeyHash, - mainUtxo, - redeemerOutputScriptP2WPKH, - requestedAmount - ) - ).to.be.revertedWith( - "There is a pending redemption request from this wallet to the same address" + // Wallet public key hash hidden under P2PKH. + await expect( + bridge + .connect(thirdParty) + .requestRedemption( + walletPubKeyHash, + mainUtxo, + `0x1976a914${walletPubKeyHash.substring(2)}88ac`, + 100000 ) - }) - } - ) - }) + ).to.be.revertedWith( + "Redeemer output script must not point to the wallet PKH" + ) - context("when amount is below the dust threshold", () => { - it("should revert", async () => { - // Initial dust threshold set in the tests `fixture` - // for tests is 100000. A value lower by 1 sat should - // trigger the tested condition. + // Wallet public key hash hidden under P2SH. await expect( bridge .connect(thirdParty) .requestRedemption( walletPubKeyHash, mainUtxo, - redeemerOutputScriptP2WPKH, - 99999 + `0x17a914${walletPubKeyHash.substring(2)}87`, + 100000 ) - ).to.be.revertedWith("Redemption amount too small") - }) - }) - } - ) + ).to.be.revertedWith( + "Redeemer output script must not point to the wallet PKH" + ) - context( - "when redeemer output script points to the wallet public key hash", - () => { - it("should revert", async () => { - // Wallet public key hash hidden under P2WPKH. - await expect( - bridge - .connect(thirdParty) - .requestRedemption( - walletPubKeyHash, - mainUtxo, - `0x160014${walletPubKeyHash.substring(2)}`, - 100000 - ) - ).to.be.revertedWith( - "Redeemer output script must not point to the wallet PKH" - ) + // There is no need to check for P2WSH since that type + // uses 32-byte hashes. Because wallet public key hash is + // always 20-byte, there is no possibility those hashes + // can be confused during change output recognition. + }) + } + ) + }) - // Wallet public key hash hidden under P2PKH. - await expect( - bridge - .connect(thirdParty) - .requestRedemption( - walletPubKeyHash, - mainUtxo, - `0x1976a914${walletPubKeyHash.substring(2)}88ac`, - 100000 - ) - ).to.be.revertedWith( - "Redeemer output script must not point to the wallet PKH" - ) + context("when redeemer output script is not standard type", () => { + it("should revert", async () => { + // The set of non-standard/malformed scripts is infinite. + // A malformed P2PKH redeemer script is used as example. + await expect( + bridge + .connect(thirdParty) + .requestRedemption( + walletPubKeyHash, + mainUtxo, + "0x1988a914f4eedc8f40d4b8e30771f792b065ebec0abaddef88ac", + 100000 + ) + ).to.be.revertedWith( + "Redeemer output script must be a standard type" + ) + }) + }) + }) - // Wallet public key hash hidden under P2SH. - await expect( - bridge - .connect(thirdParty) - .requestRedemption( - walletPubKeyHash, - mainUtxo, - `0x17a914${walletPubKeyHash.substring(2)}87`, - 100000 - ) - ).to.be.revertedWith( - "Redeemer output script must not point to the wallet PKH" - ) + context("when main UTXO data are invalid", () => { + it("should revert", async () => { + // The proper main UTXO hash `0` as `txOutputIndex`. + await expect( + bridge.connect(thirdParty).requestRedemption( + walletPubKeyHash, + { + txHash: + "0x3835ecdee2daa83c9a19b5012104ace55ecab197b5e16489c26d372e475f5d2a", + txOutputIndex: 1, + txOutputValue: 10000000, + }, + "0x160014f4eedc8f40d4b8e30771f792b065ebec0abaddef", + 100000 + ) + ).to.be.revertedWith("Invalid main UTXO data") + }) + }) + }) - // There is no need to check for P2WSH since that type - // uses 32-byte hashes. Because wallet public key hash is - // always 20-byte, there is no possibility those hashes - // can be confused during change output recognition. - }) - } - ) + context("when there is no main UTXO for the given wallet", () => { + it("should revert", async () => { + // Since there is no main UTXO for this wallet recorded in the + // Bridge, the `mainUtxo` parameter can be anything. + await expect( + bridge + .connect(thirdParty) + .requestRedemption( + walletPubKeyHash, + NO_MAIN_UTXO, + "0x160014f4eedc8f40d4b8e30771f792b065ebec0abaddef", + 100000 + ) + ).to.be.revertedWith("No main UTXO for the given wallet") }) + }) + }) + + context("when wallet state is other than Live", () => { + const testData: { testName: string; state: number }[] = [ + { + testName: "when wallet state is Unknown", + state: walletState.Unknown, + }, + { + testName: "when wallet state is MovingFunds", + state: walletState.MovingFunds, + }, + { + testName: "when wallet state is Closing", + state: walletState.Closing, + }, + { + testName: "when wallet state is Closed", + state: walletState.Closed, + }, + { + testName: "when wallet state is Terminated", + state: walletState.Terminated, + }, + ] + + testData.forEach((test) => { + context(test.testName, () => { + before(async () => { + await createSnapshot() + + await bridge.setWallet(walletPubKeyHash, { + ecdsaWalletID: ethers.constants.HashZero, + mainUtxoHash: ethers.constants.HashZero, + pendingRedemptionsValue: 0, + createdAt: await lastBlockTime(), + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: test.state, + movingFundsTargetWalletsCommitmentHash: + ethers.constants.HashZero, + }) + }) + + after(async () => { + await restoreSnapshot() + }) - context("when redeemer output script is not standard type", () => { it("should revert", async () => { - // The set of non-standard/malformed scripts is infinite. - // A malformed P2PKH redeemer script is used as example. await expect( bridge .connect(thirdParty) .requestRedemption( walletPubKeyHash, - mainUtxo, - "0x1988a914f4eedc8f40d4b8e30771f792b065ebec0abaddef88ac", + NO_MAIN_UTXO, + "0x160014f4eedc8f40d4b8e30771f792b065ebec0abaddef", 100000 ) - ).to.be.revertedWith( - "Redeemer output script must be a standard type" - ) + ).to.be.revertedWith("Wallet must be in Live state") }) }) }) + }) + }) - context("when main UTXO data are invalid", () => { - it("should revert", async () => { - // The proper main UTXO hash `0` as `txOutputIndex`. - await expect( - bridge.connect(thirdParty).requestRedemption( - walletPubKeyHash, - { - txHash: - "0x3835ecdee2daa83c9a19b5012104ace55ecab197b5e16489c26d372e475f5d2a", - txOutputIndex: 1, - txOutputValue: 10000000, - }, - "0x160014f4eedc8f40d4b8e30771f792b065ebec0abaddef", - 100000 - ) - ).to.be.revertedWith("Invalid main UTXO data") - }) + context("when redemption watchtower is set", () => { + const data: RedemptionTestData = SinglePendingRequestedRedemption + const { redeemerOutputScript, redeemer } = data.redemptionRequests[0] + + let redeemerSigner: SignerWithAddress + let watchtower: FakeContract + + before(async () => { + await createSnapshot() + + redeemerSigner = await impersonateAccount(redeemer, { + from: governance, + value: 10, }) + + watchtower = await smock.fake( + "IRedemptionWatchtower" + ) + + await bridgeGovernance + .connect(governance) + .setRedemptionWatchtower(watchtower.address) }) - context("when there is no main UTXO for the given wallet", () => { - it("should revert", async () => { - // Since there is no main UTXO for this wallet recorded in the - // Bridge, the `mainUtxo` parameter can be anything. - await expect( - bridge - .connect(thirdParty) - .requestRedemption( + after(async () => { + await restoreSnapshot() + }) + + context( + "when redemption watchtower considers the redemption as unsafe", + () => { + before(async () => { + await createSnapshot() + + watchtower.isSafeRedemption + .whenCalledWith( walletPubKeyHash, - NO_MAIN_UTXO, - "0x160014f4eedc8f40d4b8e30771f792b065ebec0abaddef", - 100000 + redeemerOutputScript, + redeemer, + redeemer ) - ).to.be.revertedWith("No main UTXO for the given wallet") - }) - }) - }) + .returns(false) + }) - context("when wallet state is other than Live", () => { - const testData: { testName: string; state: number }[] = [ - { - testName: "when wallet state is Unknown", - state: walletState.Unknown, - }, - { - testName: "when wallet state is MovingFunds", - state: walletState.MovingFunds, - }, - { - testName: "when wallet state is Closing", - state: walletState.Closing, - }, - { - testName: "when wallet state is Closed", - state: walletState.Closed, - }, - { - testName: "when wallet state is Terminated", - state: walletState.Terminated, - }, - ] + after(async () => { + watchtower.isSafeRedemption.reset() + + await restoreSnapshot() + }) - testData.forEach((test) => { - context(test.testName, () => { + it("should revert", async () => { + await expect( + bridge.connect(redeemerSigner).requestRedemption( + walletPubKeyHash, + NO_MAIN_UTXO, // not relevant + redeemerOutputScript, + 0 // not relevant + ) + ).to.be.revertedWith( + "Redemption request rejected by the watchtower" + ) + }) + } + ) + + context( + "when redemption watchtower considers the redemption as safe", + () => { before(async () => { await createSnapshot() await bridge.setWallet(walletPubKeyHash, { - ecdsaWalletID: ethers.constants.HashZero, + ecdsaWalletID: data.wallet.ecdsaWalletID, mainUtxoHash: ethers.constants.HashZero, - pendingRedemptionsValue: 0, + pendingRedemptionsValue: data.wallet.pendingRedemptionsValue, createdAt: await lastBlockTime(), movingFundsRequestedAt: 0, closingStartedAt: 0, pendingMovedFundsSweepRequestsCount: 0, - state: test.state, + state: walletState.Live, movingFundsTargetWalletsCommitmentHash: ethers.constants.HashZero, }) + await bridge.setWalletMainUtxo(walletPubKeyHash, data.mainUtxo) + await bridge.setActiveWallet(walletPubKeyHash) + + await makeRedemptionAllowance( + redeemerSigner, + data.redemptionRequests[0].amount + ) + + watchtower.isSafeRedemption + .whenCalledWith( + walletPubKeyHash, + redeemerOutputScript, + redeemer, + redeemer + ) + .returns(true) }) after(async () => { + watchtower.isSafeRedemption.reset() + await restoreSnapshot() }) - it("should revert", async () => { + it("should not revert", async () => { await expect( bridge - .connect(thirdParty) + .connect(redeemerSigner) .requestRedemption( walletPubKeyHash, - NO_MAIN_UTXO, - "0x160014f4eedc8f40d4b8e30771f792b065ebec0abaddef", - 100000 + data.mainUtxo, + redeemerOutputScript, + data.redemptionRequests[0].amount ) - ).to.be.revertedWith("Wallet must be in Live state") + ).to.not.be.reverted }) - }) - }) + } + ) }) }) diff --git a/solidity/test/bridge/RedemptionWatchtower.test.ts b/solidity/test/bridge/RedemptionWatchtower.test.ts index e7f68e720..01380ff49 100644 --- a/solidity/test/bridge/RedemptionWatchtower.test.ts +++ b/solidity/test/bridge/RedemptionWatchtower.test.ts @@ -1577,6 +1577,146 @@ describe("RedemptionWatchtower", () => { }) }) + describe("isSafeRedemption", () => { + let vetoedRedemption: RedemptionData + let objectedNonVetoedRedemption: RedemptionData + + before(async () => { + await createSnapshot() + + // eslint-disable-next-line prefer-destructuring + vetoedRedemption = ( + await createRedemptionRequests(SinglePendingRequestedRedemption) + )[0] + + // Create another redemption using the same SinglePendingRequestedRedemption + // data. Use different redeemerOutputScript to avoid collision + // with the first redemption. + const redemptionData = JSON.parse( + JSON.stringify(SinglePendingRequestedRedemption) + ) + redemptionData.redemptionRequests[0].redeemerOutputScript = + "0x17a914011beb6fb8499e075a57027fb0a58384f2d3f78487" + // eslint-disable-next-line prefer-destructuring + objectedNonVetoedRedemption = ( + await createRedemptionRequests(redemptionData) + )[0] + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + + // Raise three objections to veto the first redemption and ban the redeemer. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + vetoedRedemption.walletPublicKeyHash, + vetoedRedemption.redeemerOutputScript + ) + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + vetoedRedemption.walletPublicKeyHash, + vetoedRedemption.redeemerOutputScript + ) + await redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + vetoedRedemption.walletPublicKeyHash, + vetoedRedemption.redeemerOutputScript + ) + + // Raise a single objection to the second "objected but non-vetoed" redemption. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + objectedNonVetoedRedemption.walletPublicKeyHash, + objectedNonVetoedRedemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the balance owner is banned", () => { + it("should return false", async () => { + // Check non-objected redemption with the banned redeemer as balance owner. + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.isSafeRedemption( + "0x7ac2d9378a1c47e589dfb8095ca95ed2140d2726", + "0x1976a9142cd680318747b720d67bf4246eb7403b476adb3488ac", + vetoedRedemption.redeemer, + "0x0Bf9bD12462c43A91F13440faF9f9BD6ece37689" + ) + ).to.be.false + }) + }) + + context("when the redeemer is banned", () => { + it("should return false", async () => { + // Check non-objected redemption with the banned redeemer as redeemer. + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.isSafeRedemption( + "0x7ac2d9378a1c47e589dfb8095ca95ed2140d2726", + "0x1976a9142cd680318747b720d67bf4246eb7403b476adb3488ac", + "0x0Bf9bD12462c43A91F13440faF9f9BD6ece37689", + vetoedRedemption.redeemer + ) + ).to.be.false + }) + }) + + context("when redemption key was vetoed", () => { + it("should return false", async () => { + // Check vetoed redemption with non-banned balance owner and redeemer. + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.isSafeRedemption( + vetoedRedemption.walletPublicKeyHash, + vetoedRedemption.redeemerOutputScript, + "0x0Bf9bD12462c43A91F13440faF9f9BD6ece37689", + "0x90a4ac843763F7F345f2738CcC9F420D59751249" + ) + ).to.be.false + }) + }) + + context("when redemption key was objected but not vetoed", () => { + it("should return false", async () => { + // Check objected but non-vetoed redemption with non-banned balance + // owner and redeemer. + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.isSafeRedemption( + objectedNonVetoedRedemption.walletPublicKeyHash, + objectedNonVetoedRedemption.redeemerOutputScript, + "0x0Bf9bD12462c43A91F13440faF9f9BD6ece37689", + "0x90a4ac843763F7F345f2738CcC9F420D59751249" + ) + ).to.be.false + }) + }) + + context("when all safety criteria are met", () => { + it("should return true", async () => { + // Check non-objected redemption with non-banned balance owner and redeemer. + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.isSafeRedemption( + "0x7ac2d9378a1c47e589dfb8095ca95ed2140d2726", + "0x1976a9142cd680318747b720d67bf4246eb7403b476adb3488ac", + "0x0Bf9bD12462c43A91F13440faF9f9BD6ece37689", + "0x90a4ac843763F7F345f2738CcC9F420D59751249" + ) + ).to.be.true + }) + }) + }) + type RedemptionData = { redemptionKey: string walletPublicKeyHash: string