From b42f0312905f07db7714d061878f58b08a2d2121 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Mon, 19 Feb 2024 13:40:17 +0100 Subject: [PATCH 01/23] Introduce the `RedemptionWatchtower` contract Here we introduce the `RedemptionWatchtower` contract that will encapsulate the logic of the redemption veto mechanism. This contract will be attached to the `Bridge` contract upon the upcoming upgrade. --- solidity/contracts/bridge/Bridge.sol | 20 ++++ .../contracts/bridge/BridgeGovernance.sol | 16 +++ solidity/contracts/bridge/BridgeState.sol | 30 ++++- .../contracts/bridge/RedemptionWatchtower.sol | 46 ++++++++ .../test/bridge/Bridge.Governance.test.ts | 37 +++++++ .../test/bridge/Bridge.Parameters.test.ts | 103 +++++++++++++++++- 6 files changed, 248 insertions(+), 4 deletions(-) create mode 100644 solidity/contracts/bridge/RedemptionWatchtower.sol diff --git a/solidity/contracts/bridge/Bridge.sol b/solidity/contracts/bridge/Bridge.sol index f1395e04e..e14378e52 100644 --- a/solidity/contracts/bridge/Bridge.sol +++ b/solidity/contracts/bridge/Bridge.sol @@ -236,6 +236,8 @@ contract Bridge is event TreasuryUpdated(address treasury); + event RedemptionWatchtowerSet(address redemptionWatchtower); + modifier onlySpvMaintainer() { require( self.isSpvMaintainer[msg.sender], @@ -1944,4 +1946,22 @@ contract Bridge is function txProofDifficultyFactor() external view returns (uint256) { return self.txProofDifficultyFactor; } + + /// @notice Sets the redemption watchtower address. + /// @param redemptionWatchtower Address of the redemption watchtower. + /// @dev Requirements: + /// - The caller must be the governance, + /// - Redemption watchtower address must not be already set, + /// - Redemption watchtower address must not be 0x0. + function setRedemptionWatchtower(address redemptionWatchtower) + external + onlyGovernance + { + self.setRedemptionWatchtower(redemptionWatchtower); + } + + /// @return Address of the redemption watchtower. + function redemptionWatchtower() external view returns (address) { + return self.redemptionWatchtower; + } } diff --git a/solidity/contracts/bridge/BridgeGovernance.sol b/solidity/contracts/bridge/BridgeGovernance.sol index 076b85b3e..2126bcd8b 100644 --- a/solidity/contracts/bridge/BridgeGovernance.sol +++ b/solidity/contracts/bridge/BridgeGovernance.sol @@ -1766,4 +1766,20 @@ contract BridgeGovernance is Ownable { function governanceDelay() internal view returns (uint256) { return governanceDelays[0]; } + + /// @notice Sets the redemption watchtower address. This function does not + /// have a governance delay as setting the redemption watchtower is + /// a one-off action performed during initialization of the + /// redemption veto mechanism. + /// @param redemptionWatchtower Address of the redemption watchtower. + /// @dev Requirements: + /// - The caller must be the owner, + /// - Redemption watchtower address must not be already set, + /// - Redemption watchtower address must not be 0x0. + function setRedemptionWatchtower(address redemptionWatchtower) + external + onlyOwner + { + bridge.setRedemptionWatchtower(redemptionWatchtower); + } } diff --git a/solidity/contracts/bridge/BridgeState.sol b/solidity/contracts/bridge/BridgeState.sol index 0b4a7fc70..277d8c52d 100644 --- a/solidity/contracts/bridge/BridgeState.sol +++ b/solidity/contracts/bridge/BridgeState.sol @@ -314,6 +314,9 @@ library BridgeState { // HASH160 over the compressed ECDSA public key) to the basic wallet // information like state and pending redemptions value. mapping(bytes20 => Wallets.Wallet) registeredWallets; + // Address of the redemption watchtower. The redemption watchtower + // is responsible for vetoing redemption requests. + address redemptionWatchtower; // Reserved storage space in case we need to add more variables. // The convention from OpenZeppelin suggests the storage space should // add up to 50 slots. Here we want to have more slots as there are @@ -321,7 +324,7 @@ library BridgeState { // the struct in the upcoming versions we need to reduce the array size. // See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps // slither-disable-next-line unused-state - uint256[50] __gap; + uint256[49] __gap; } event DepositParametersUpdated( @@ -374,6 +377,8 @@ library BridgeState { event TreasuryUpdated(address treasury); + event RedemptionWatchtowerSet(address redemptionWatchtower); + /// @notice Updates parameters of deposits. /// @param _depositDustThreshold New value of the deposit dust threshold in /// satoshis. It is the minimal amount that can be requested to @@ -826,4 +831,27 @@ library BridgeState { self.treasury = _treasury; emit TreasuryUpdated(_treasury); } + + /// @notice Sets the redemption watchtower address. + /// @param _redemptionWatchtower Address of the redemption watchtower. + /// @dev Requirements: + /// - Redemption watchtower address must not be already set, + /// - Redemption watchtower address must not be 0x0. + function setRedemptionWatchtower( + Storage storage self, + address _redemptionWatchtower + ) internal { + require( + self.redemptionWatchtower == address(0), + "Redemption watchtower already set" + ); + + require( + _redemptionWatchtower != address(0), + "Redemption watchtower address must not be 0x0" + ); + + self.redemptionWatchtower = _redemptionWatchtower; + emit RedemptionWatchtowerSet(_redemptionWatchtower); + } } diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol new file mode 100644 index 000000000..c299ed38e --- /dev/null +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: GPL-3.0-only + +// ██████████████ ▐████▌ ██████████████ +// ██████████████ ▐████▌ ██████████████ +// ▐████▌ ▐████▌ +// ▐████▌ ▐████▌ +// ██████████████ ▐████▌ ██████████████ +// ██████████████ ▐████▌ ██████████████ +// ▐████▌ ▐████▌ +// ▐████▌ ▐████▌ +// ▐████▌ ▐████▌ +// ▐████▌ ▐████▌ +// ▐████▌ ▐████▌ +// ▐████▌ ▐████▌ + +pragma solidity 0.8.17; + +import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; + +import "./Bridge.sol"; + +/// @title Redemption watchtower +/// @notice This contract encapsulates the logic behind the redemption veto +/// mechanism of the Bridge. The redemption veto mechanism is a safeguard +/// in the event of malicious redemption requests such as those sourced from a +/// Bridge hack. The mechanism involves a permissioned set of Guardians +/// that can object to a redemption request. If a redemption is objected to by +/// an optimistic redemption guardian, it is delayed. Two subsequent objections +/// from redemption guardians results in a veto of the redemption request. +/// A veto returns redeemed amount to the requester while inflicting a freeze +/// penalty on the amount and a financial penalty. The goal of this penalty is +/// to introduce a cost that guards against repeated malicious redemption requests. +contract RedemptionWatchtower is OwnableUpgradeable { + Bridge public bridge; + + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + function initialize(Bridge _bridge) external initializer { + __Ownable_init(); + + bridge = _bridge; + } +} diff --git a/solidity/test/bridge/Bridge.Governance.test.ts b/solidity/test/bridge/Bridge.Governance.test.ts index 1c8c0f344..39ccb9ce1 100644 --- a/solidity/test/bridge/Bridge.Governance.test.ts +++ b/solidity/test/bridge/Bridge.Governance.test.ts @@ -4425,4 +4425,41 @@ describe("Bridge - Governance", () => { }) }) }) + + describe("setRedemptionWatchtower", () => { + const watchtower = "0xE8ebaEc51bAeeaBff71707dE2AD028C7fB642A3F" + + context("when caller is not the owner", () => { + it("should revert", async () => { + await expect( + bridgeGovernance + .connect(thirdParty) + .setRedemptionWatchtower(watchtower) + ).to.be.revertedWith("Ownable: caller is not the owner") + }) + }) + + context("when caller is the owner", () => { + let tx: Promise + + before(async () => { + await createSnapshot() + + tx = bridgeGovernance + .connect(governance) + .setRedemptionWatchtower(watchtower) + }) + + after(async () => { + await restoreSnapshot() + }) + + // Detailed tests covering the `bridge.setRedemptionWatchtower` call + // can be found in the `Bridge.Parameters.test.ts` file. Here we just + // ensure correctness of the BridgeGovernance's ACL. + it("should not revert", async () => { + await expect(tx).to.not.be.reverted + }) + }) + }) }) diff --git a/solidity/test/bridge/Bridge.Parameters.test.ts b/solidity/test/bridge/Bridge.Parameters.test.ts index e5f428482..9476303de 100644 --- a/solidity/test/bridge/Bridge.Parameters.test.ts +++ b/solidity/test/bridge/Bridge.Parameters.test.ts @@ -1759,11 +1759,12 @@ describe("Bridge - Parameters", () => { context("when caller is the contract guvnor", () => { before(async () => { - // TODO: We transfer the ownership of the Bridge governance from the + await createSnapshot() + + // We transfer the ownership of the Bridge governance from the // BridgeGovernance contract to a simple address. This allows testing // the Bridge contract directly, without going through the - // BridgeGovernance contract. This should be the preferred approach for - // all other tests in this file. + // BridgeGovernance contract. await bridgeGovernance .connect(governance) .beginBridgeGovernanceTransfer(governance.address) @@ -1773,6 +1774,10 @@ describe("Bridge - Parameters", () => { .finalizeBridgeGovernanceTransfer() }) + after(async () => { + await restoreSnapshot() + }) + context("when the new treasury address is non-zero", () => { let tx: ContractTransaction @@ -1814,4 +1819,96 @@ describe("Bridge - Parameters", () => { }) }) }) + + describe("setRedemptionWatchtower", () => { + const watchtower = "0xE8ebaEc51bAeeaBff71707dE2AD028C7fB642A3F" + + context("when caller is not the contract guvnor", () => { + it("should revert", async () => { + await expect( + bridge.connect(thirdParty).setRedemptionWatchtower(watchtower) + ).to.be.revertedWith("Caller is not the governance") + }) + }) + + context("when caller is the contract guvnor", () => { + before(async () => { + await createSnapshot() + + // We transfer the ownership of the Bridge governance from the + // BridgeGovernance contract to a simple address. This allows testing + // the Bridge contract directly, without going through the + // BridgeGovernance contract. + await bridgeGovernance + .connect(governance) + .beginBridgeGovernanceTransfer(governance.address) + await helpers.time.increaseTime(constants.governanceDelay) + await bridgeGovernance + .connect(governance) + .finalizeBridgeGovernanceTransfer() + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the watchtower address is already set", () => { + before(async () => { + await createSnapshot() + + await bridge.connect(governance).setRedemptionWatchtower(watchtower) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + bridge + .connect(governance) + .setRedemptionWatchtower(thirdParty.address) + ).to.be.revertedWith("Redemption watchtower already set") + }) + }) + + context("when the watchtower address is not set yet", () => { + context("when the watchtower address is zero", () => { + it("should revert", async () => { + await expect( + bridge.connect(governance).setRedemptionWatchtower(ZERO_ADDRESS) + ).to.be.revertedWith( + "Redemption watchtower address must not be 0x0" + ) + }) + }) + + context("when the watchtower address is non-zero", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + tx = await bridge + .connect(governance) + .setRedemptionWatchtower(watchtower) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should set the watchtower address", async () => { + expect(await bridge.redemptionWatchtower()).to.equal(watchtower) + }) + + it("should emit RedemptionWatchtowerSet event", async () => { + await expect(tx) + .to.emit(bridge, "RedemptionWatchtowerSet") + .withArgs(watchtower) + }) + }) + }) + }) + }) }) From c97e87350b13f873351c7ea66e83676795659276 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Mon, 19 Feb 2024 14:49:00 +0100 Subject: [PATCH 02/23] Turn on the watchtower and manage guardians Here we implement functions allowing to enable the watchtower (i.e. veto mechanism) and manage guardians. The `enableWatchtower` function allows setting the watchtower manager and an initial set of guardians. Moreover, we are capturing the initiation timestamp which will be useful while determining pre-OR and post-OR redemption requests and will be needed to implement the planned lifecycle of the veto mechanism Regarding guardian management, we are adding two functions: - `addGuardian` that can be called by the watchtower manager (Token Holder DAO) - `removeGuardian` that can be called by the owner (Threshold Council) --- .../contracts/bridge/RedemptionWatchtower.sol | 95 +++++- .../deploy/40_deploy_redemption_watchtower.ts | 43 +++ ...ransfer_redemption_watchtower_ownership.ts | 19 ++ solidity/hardhat.config.ts | 5 + .../test/bridge/RedemptionWatchtower.test.ts | 282 ++++++++++++++++++ solidity/test/fixtures/bridge.ts | 22 +- 6 files changed, 454 insertions(+), 12 deletions(-) create mode 100644 solidity/deploy/40_deploy_redemption_watchtower.ts create mode 100644 solidity/deploy/41_transfer_redemption_watchtower_ownership.ts create mode 100644 solidity/test/bridge/RedemptionWatchtower.test.ts diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index c299ed38e..6d1b624f3 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -21,17 +21,37 @@ import "./Bridge.sol"; /// @title Redemption watchtower /// @notice This contract encapsulates the logic behind the redemption veto -/// mechanism of the Bridge. The redemption veto mechanism is a safeguard -/// in the event of malicious redemption requests such as those sourced from a -/// Bridge hack. The mechanism involves a permissioned set of Guardians -/// that can object to a redemption request. If a redemption is objected to by -/// an optimistic redemption guardian, it is delayed. Two subsequent objections -/// from redemption guardians results in a veto of the redemption request. -/// A veto returns redeemed amount to the requester while inflicting a freeze -/// penalty on the amount and a financial penalty. The goal of this penalty is -/// to introduce a cost that guards against repeated malicious redemption requests. +/// mechanism of the Bridge. The redemption veto mechanism is a safeguard +/// in the event of malicious redemption requests such as those sourced +/// from a Bridge hack. The mechanism involves a permissioned set of +/// Guardians that can object to a redemption request. If a redemption +/// is objected to by a redemption guardian, it is delayed. Two +/// subsequent objections from redemption guardians results in a veto +/// of the redemption request. A veto returns redeemed amount to the +/// requester while inflicting a freeze and financial penalty on the +/// amount. The goal of this penalty is to introduce a cost that guards +/// against repeated malicious redemption requests. contract RedemptionWatchtower is OwnableUpgradeable { + /// @notice Set of redemption guardians. + mapping(address => bool) public isGuardian; + /// @notice The Bridge contract. Bridge public bridge; + // UNIX timestamp the redemption watchtower (and veto mechanism) was enabled at. + // XXX: Unsigned 32-bit int unix seconds, will break February 7th 2106. + uint32 public watchtowerEnabledAt; + /// @notice Address of the manager responsible for parameters management. + address public manager; + + event WatchtowerEnabled(uint32 enabledAt, address manager); + + event GuardianAdded(address indexed guardian); + + event GuardianRemoved(address indexed guardian); + + modifier onlyManager() { + require(msg.sender == manager, "Caller is not watchtower manager"); + _; + } /// @custom:oz-upgrades-unsafe-allow constructor constructor() { @@ -43,4 +63,61 @@ contract RedemptionWatchtower is OwnableUpgradeable { bridge = _bridge; } + + /// @notice Enables the redemption watchtower and veto mechanism. + /// @param _manager Address of the watchtower manager. + /// @param _guardians List of initial guardian addresses. + /// @dev Requirements: + /// - The caller must be the owner, + /// - Watchtower must not be enabled already, + /// - Manager address must not be zero. + function enableWatchtower(address _manager, address[] calldata _guardians) + external + onlyOwner + { + require(watchtowerEnabledAt == 0, "Already enabled"); + + require(_manager != address(0), "Manager address must not be 0x0"); + manager = _manager; + + for (uint256 i = 0; i < _guardians.length; i++) { + _addGuardian(_guardians[i]); + } + + /* solhint-disable-next-line not-rely-on-time */ + uint32 enabledAt = uint32(block.timestamp); + watchtowerEnabledAt = enabledAt; + + emit WatchtowerEnabled(enabledAt, _manager); + } + + /// @notice Adds a redemption guardian + /// @param guardian Address of the guardian to add. + /// @dev Requirements: + /// - The caller must be the watchtower manager, + /// - The guardian must not already exist. + function addGuardian(address guardian) external onlyManager { + _addGuardian(guardian); + } + + /// @notice Adds a redemption guardian + /// @param guardian Address of the guardian to add. + /// @dev Requirements: + /// - The guardian must not already exist. + function _addGuardian(address guardian) internal { + require(!isGuardian[guardian], "Guardian already exists"); + isGuardian[guardian] = true; + emit GuardianAdded(guardian); + } + + /// @notice Removes a redemption guardian + /// @param guardian Address of the guardian to remove. + /// @dev Requirements: + /// - The caller must be the owner, + /// - The guardian must exist. + function removeGuardian(address guardian) external onlyOwner { + require(isGuardian[guardian], "Guardian does not exist"); + delete isGuardian[guardian]; + emit GuardianRemoved(guardian); + } } diff --git a/solidity/deploy/40_deploy_redemption_watchtower.ts b/solidity/deploy/40_deploy_redemption_watchtower.ts new file mode 100644 index 000000000..60495d07e --- /dev/null +++ b/solidity/deploy/40_deploy_redemption_watchtower.ts @@ -0,0 +1,43 @@ +import { HardhatRuntimeEnvironment } from "hardhat/types" +import { DeployFunction } from "hardhat-deploy/types" + +const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { + const { deployments, ethers, helpers, getNamedAccounts } = hre + const { deployer } = await getNamedAccounts() + + const Bridge = await deployments.get("Bridge") + + const [redemptionWatchtower, proxyDeployment] = + await helpers.upgrades.deployProxy("RedemptionWatchtower", { + contractName: "RedemptionWatchtower", + initializerArgs: [Bridge.address], + factoryOpts: { + signer: await ethers.getSigner(deployer), + }, + proxyOpts: { + kind: "transparent", + }, + }) + + if (hre.network.tags.etherscan) { + // We use `verify` instead of `verify:verify` as the `verify` task is defined + // in "@openzeppelin/hardhat-upgrades" to perform Etherscan verification + // of Proxy and Implementation contracts. + await hre.run("verify", { + address: proxyDeployment.address, + constructorArgsParams: proxyDeployment.args, + }) + } + + if (hre.network.tags.tenderly) { + await hre.tenderly.verify({ + name: "RedemptionWatchtower", + address: redemptionWatchtower.address, + }) + } +} + +export default func + +func.tags = ["RedemptionWatchtower"] +func.dependencies = ["Bridge"] diff --git a/solidity/deploy/41_transfer_redemption_watchtower_ownership.ts b/solidity/deploy/41_transfer_redemption_watchtower_ownership.ts new file mode 100644 index 000000000..3ca46dc8a --- /dev/null +++ b/solidity/deploy/41_transfer_redemption_watchtower_ownership.ts @@ -0,0 +1,19 @@ +import { HardhatRuntimeEnvironment } from "hardhat/types" +import { DeployFunction } from "hardhat-deploy/types" + +const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { + const { getNamedAccounts, helpers } = hre + const { deployer, governance } = await getNamedAccounts() + + await helpers.ownable.transferOwnership( + "RedemptionWatchtower", + governance, + deployer + ) +} + +export default func + +func.tags = ["RedemptionWatchtowerOwnership"] +func.dependencies = ["RedemptionWatchtower"] +func.runAtTheEnd = true diff --git a/solidity/hardhat.config.ts b/solidity/hardhat.config.ts index 93ac97c18..2eef2dcf2 100644 --- a/solidity/hardhat.config.ts +++ b/solidity/hardhat.config.ts @@ -224,6 +224,11 @@ const config: HardhatUserConfig = { sepolia: 0, mainnet: "0x8Bac178fA95Cb56D11A94d4f1b2B1F5Fc48A30eA", }, + redemptionWatchtowerManager: { + default: 11, + sepolia: 0, + mainnet: "0x87F005317692D05BAA4193AB0c961c69e175f45f", // Token Holder DAO + }, }, dependencyCompiler: { paths: [ diff --git a/solidity/test/bridge/RedemptionWatchtower.test.ts b/solidity/test/bridge/RedemptionWatchtower.test.ts new file mode 100644 index 000000000..4d1d396cb --- /dev/null +++ b/solidity/test/bridge/RedemptionWatchtower.test.ts @@ -0,0 +1,282 @@ +import { helpers, waffle, ethers } from "hardhat" +import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers" +import { expect } from "chai" +import { ContractTransaction } from "ethers" +import type { Bridge, BridgeStub, RedemptionWatchtower } from "../../typechain" +import bridgeFixture from "../fixtures/bridge" + +const { createSnapshot, restoreSnapshot } = helpers.snapshot +const { lastBlockTime } = helpers.time + +describe("RedemptionWatchtower", () => { + let governance: SignerWithAddress + let thirdParty: SignerWithAddress + let redemptionWatchtowerManager: SignerWithAddress + let guardians: SignerWithAddress[] + + let bridge: Bridge & BridgeStub + let redemptionWatchtower: RedemptionWatchtower + + before(async () => { + // eslint-disable-next-line @typescript-eslint/no-extra-semi + ;({ + governance, + thirdParty, + redemptionWatchtowerManager, + guardians, + bridge, + redemptionWatchtower, + } = await waffle.loadFixture(bridgeFixture)) + + // Make sure test actors are correctly set up. + const actors = [ + governance, + thirdParty, + redemptionWatchtowerManager, + ...guardians, + ].map((actor) => actor.address) + + if (actors.length !== new Set(actors).size) { + throw new Error("Duplicate actors; please double check the fixture") + } + }) + + describe("enableWatchtower", () => { + context("when called not by the owner", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower.connect(thirdParty).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + ).to.be.revertedWith("Ownable: caller is not the owner") + }) + }) + + context("when called by the owner", () => { + context("when already enabled", () => { + before(async () => { + await createSnapshot() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + ).to.be.revertedWith("Already enabled") + }) + }) + + context("when not enabled yet", () => { + context("when manager address is zero", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower.connect(governance).enableWatchtower( + ethers.constants.AddressZero, + guardians.map((g) => g.address) + ) + ).to.be.revertedWith("Manager address must not be 0x0") + }) + }) + + context("when manager address is non-zero", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + tx = await redemptionWatchtower + .connect(governance) + .enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should set the watchtower manager properly", async () => { + expect(await redemptionWatchtower.manager()).to.equal( + redemptionWatchtowerManager.address + ) + }) + + it("should set initial guardians properly", async () => { + // eslint-disable-next-line no-restricted-syntax + for (const guardian of guardians) { + // eslint-disable-next-line no-await-in-loop,@typescript-eslint/no-unused-expressions + expect(await redemptionWatchtower.isGuardian(guardian.address)).to + .be.true + } + }) + + it("should emit WatchtowerEnabled event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "WatchtowerEnabled") + .withArgs( + await lastBlockTime(), + redemptionWatchtowerManager.address + ) + }) + + it("should emit GuardianAdded events", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "GuardianAdded") + .withArgs(guardians[0].address) + + await expect(tx) + .to.emit(redemptionWatchtower, "GuardianAdded") + .withArgs(guardians[1].address) + + await expect(tx) + .to.emit(redemptionWatchtower, "GuardianAdded") + .withArgs(guardians[2].address) + }) + }) + }) + }) + }) + + describe("addGuardian", () => { + before(async () => { + await createSnapshot() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when called not by the watchtower manager", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(governance) // governance has not such a power + .addGuardian(thirdParty.address) + ).to.be.revertedWith("Caller is not watchtower manager") + }) + }) + + context("when called by the watchtower manager", () => { + context("when guardian already exists", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redemptionWatchtowerManager) + .addGuardian(guardians[0].address) + ).to.be.revertedWith("Guardian already exists") + }) + }) + + context("when guardian does not exist", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + tx = await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .addGuardian(thirdParty.address) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should add the guardian properly", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect(await redemptionWatchtower.isGuardian(thirdParty.address)).to + .be.true + }) + + it("should emit GuardianAdded event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "GuardianAdded") + .withArgs(thirdParty.address) + }) + }) + }) + }) + + describe("removeGuardian", () => { + before(async () => { + await createSnapshot() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when called not by the governance", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redemptionWatchtowerManager) // manager has not such a power + .removeGuardian(guardians[0].address) + ).to.be.revertedWith("Ownable: caller is not the owner") + }) + }) + + context("when called by the governance", () => { + context("when guardian does not exist", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(governance) + .removeGuardian(thirdParty.address) + ).to.be.revertedWith("Guardian does not exist") + }) + }) + + context("when guardian exists", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + tx = await redemptionWatchtower + .connect(governance) + .removeGuardian(guardians[0].address) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should remove the guardian properly", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect(await redemptionWatchtower.isGuardian(guardians[0].address)).to + .be.false + }) + + it("should emit GuardianRemoved event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "GuardianRemoved") + .withArgs(guardians[0].address) + }) + }) + }) + }) +}) diff --git a/solidity/test/fixtures/bridge.ts b/solidity/test/fixtures/bridge.ts index 7053ac1c1..5072844c2 100644 --- a/solidity/test/fixtures/bridge.ts +++ b/solidity/test/fixtures/bridge.ts @@ -14,6 +14,7 @@ import type { VendingMachine, BridgeGovernance, IRelay, + RedemptionWatchtower, } from "../../typechain" /** @@ -22,9 +23,18 @@ import type { export default async function bridgeFixture() { await deployments.fixture() - const { deployer, governance, spvMaintainer, treasury } = - await helpers.signers.getNamedSigners() - const [thirdParty] = await helpers.signers.getUnnamedSigners() + const { + deployer, + governance, + spvMaintainer, + treasury, + redemptionWatchtowerManager, + } = await helpers.signers.getNamedSigners() + + const [thirdParty, guardian1, guardian2, guardian3] = + await helpers.signers.getUnnamedSigners() + + const guardians = [guardian1, guardian2, guardian3] const tbtc: TBTC = await helpers.contracts.getContract("TBTC") @@ -66,6 +76,9 @@ export default async function bridgeFixture() { await bank.connect(governance).updateBridge(bridge.address) + const redemptionWatchtower: RedemptionWatchtower = + await helpers.contracts.getContract("RedemptionWatchtower") + // Deploys a new instance of Bridge contract behind a proxy. Allows to // specify txProofDifficultyFactor. The new instance is deployed with // a random name to do not conflict with the main deployed instance. @@ -111,6 +124,8 @@ export default async function bridgeFixture() { spvMaintainer, thirdParty, treasury, + redemptionWatchtowerManager, + guardians, tbtc, vendingMachine, tbtcVault, @@ -121,6 +136,7 @@ export default async function bridgeFixture() { reimbursementPool, maintainerProxy, bridgeGovernance, + redemptionWatchtower, deployBridge, } } From 941cf9fea61295e8684ec153252a1fd5a4a4273f Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Mon, 19 Feb 2024 15:22:59 +0100 Subject: [PATCH 03/23] Add `RedemptionWatchtower` state Here we add all the state variables that must be implemented as part of the `RedemptionWatchtower` contract. Specific variables and types were determined during the prototype implementation phase. --- .../contracts/bridge/RedemptionWatchtower.sol | 85 ++++++++++++++++++- 1 file changed, 83 insertions(+), 2 deletions(-) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index 6d1b624f3..d5876d830 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -32,15 +32,89 @@ import "./Bridge.sol"; /// amount. The goal of this penalty is to introduce a cost that guards /// against repeated malicious redemption requests. contract RedemptionWatchtower is OwnableUpgradeable { + struct VetoProposal { + // Address of the redeemer that requested the redemption. + address redeemer; + // Amount that the redeemer can claim after the freeze period. + // Value is 0 if the veto is not finalized or the amount was + // already claimed. + uint64 claimableAmount; + // Timestamp when the veto was finalized. Value is 0 if the veto is + // not finalized. + uint32 finalizedAt; + // Number of objections raised against the redemption request. + uint8 objectionsCount; + } + /// @notice Set of redemption guardians. mapping(address => bool) public isGuardian; + /// @notice Set of banned redeemer addresses. Banned redeemers cannot + /// request redemptions. A redeemer is banned if one of their + /// redemption requests is vetoed due to an enough number of + /// guardian objections. + mapping(address => bool) public isBanned; + /// @notice Set of veto proposals indexed by the redemption key built as + /// `keccak256(keccak256(redeemerOutputScript) | walletPubKeyHash)`. + /// The `walletPubKeyHash` is the 20-byte wallet's public key hash + /// (computed using Bitcoin HASH160 over the compressed ECDSA + /// public key) and `redeemerOutputScript` is the Bitcoin script + /// (P2PKH, P2WPKH, P2SH or P2WSH) that is involved in the + /// redemption request. + mapping(uint256 => VetoProposal) public vetoProposals; + /// @notice Set of individual guardian objections indexed by the objection + /// key built as `keccak256(redemptionKey | guardian)`. + /// The `redemptionKey` is the redemption key built in the same way + /// as in the `vetoProposals` mapping. The `guardian` is the + /// address of the guardian who raised the objection. + mapping(uint256 => bool) public objections; /// @notice The Bridge contract. Bridge public bridge; - // UNIX timestamp the redemption watchtower (and veto mechanism) was enabled at. - // XXX: Unsigned 32-bit int unix seconds, will break February 7th 2106. + /// @notice UNIX timestamp the redemption watchtower (and veto mechanism) + /// was enabled at. uint32 public watchtowerEnabledAt; + /// @notice Duration of the watchtower lifetime in seconds. Once this + /// period elapses (since the `watchtowerEnabledAt` timestamp), + /// the watchtower can be permanently disabled by anyone. + uint32 public watchtowerLifetime; + /// @notice UNIX timestamp the redemption watchtower (and veto mechanism) + /// was permanently disabled at. + uint32 public watchtowerDisabledAt; /// @notice Address of the manager responsible for parameters management. address public manager; + /// @notice Divisor used to compute the redemption veto penalty fee deducted + /// upon veto finalization. This fee diminishes the amount that the + /// redeemer can claim after the freeze period and is computed as follows: + /// `penaltyFee = requestedAmount / redemptionVetoPenaltyFeeDivisor` + /// For example, if the penalty fee needs to be 2% of each vetoed + /// redemption request, the `redemptionVetoPenaltyFeeDivisor` should + /// be set to `50` because `1/50 = 0.02 = 2%`. + uint64 public vetoPenaltyFeeDivisor; + /// @notice Time of the redemption veto freeze period. It is the time after + /// which the redeemer can claim the amount of the vetoed redemption + /// request. The freeze period is counted from the moment when the + /// veto request was finalized (i.e. moment of the last guardian + /// objection that caused finalization). Value in seconds. + uint32 public vetoFreezePeriod; + /// @notice Default delay applied to each redemption request. It is the time + /// during which redemption guardians can raise the first objection. + /// Wallets are not allowed to finalize the redemption request before + /// the delay is over. The delay is counted from the moment when the + /// redemption request was created. Value in seconds. + uint32 public defaultDelay; + /// @notice Delay applied to redemption requests a single guardian raised an + /// objection to. It is the time during which the remaining guardians + /// can raise their objections. Wallets are not allowed to finalize the + /// redemption request before the delay is over. The delay is counted + /// from the moment when the redemption request was created. + /// Value in seconds. + uint32 public levelOneDelay; + /// @notice Delay applied to redemption requests two guardians raised an + /// objection to. It is the time during which the last guardian + /// can raise its objection. Wallets are not allowed to finalize the + /// redemption request before the delay is over. The delay is counted + /// from the moment when the redemption request was created. + /// Value in seconds. + uint32 public levelTwoDelay; event WatchtowerEnabled(uint32 enabledAt, address manager); @@ -62,6 +136,13 @@ contract RedemptionWatchtower is OwnableUpgradeable { __Ownable_init(); bridge = _bridge; + + watchtowerLifetime = 18 * 30 days; // 18 months + vetoPenaltyFeeDivisor = 1; // 100% as initial penalty fee + vetoFreezePeriod = 30 days; // 1 month + defaultDelay = 2 hours; + levelOneDelay = 8 hours; + levelTwoDelay = 24 hours; } /// @notice Enables the redemption watchtower and veto mechanism. From 9fe9d9a61f4e342dd050bbba3ff952405eb48543 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Mon, 19 Feb 2024 17:48:03 +0100 Subject: [PATCH 04/23] Objection mechanism Here we expose the `raiseObjection` function that allows guardians object to specific redemption requests. Each redemption has a default delay period during which the wallet is not allowed to process it and the guardians can raise objections to. Each objection extends the delay period by a certain amount of time. The third objection vetoes the redemption request. This causes the redemption request to be rejected and the redeemer to be penalized. Specific consequences of a veto are as follows: - The redemption amount is frozen for a certain period of time, - Once the freeze period expires, the redeemer can claim the frozen amount minus a penalty fee, - The penalty fee is burned, - The redeemer is banned from making future redemption requests. --- solidity/contracts/bridge/Bridge.sol | 25 +++ solidity/contracts/bridge/Redemption.sol | 62 ++++++++ .../contracts/bridge/RedemptionWatchtower.sol | 147 ++++++++++++++++++ .../bridge/WalletProposalValidator.sol | 2 + 4 files changed, 236 insertions(+) diff --git a/solidity/contracts/bridge/Bridge.sol b/solidity/contracts/bridge/Bridge.sol index e14378e52..9f85e31c0 100644 --- a/solidity/contracts/bridge/Bridge.sol +++ b/solidity/contracts/bridge/Bridge.sol @@ -1964,4 +1964,29 @@ contract Bridge is function redemptionWatchtower() external view returns (address) { return self.redemptionWatchtower; } + + /// @notice Notifies that a redemption request was vetoed in the watchtower. + /// This function is responsible for adjusting the Bridge's state + /// accordingly. + /// The results of calling this function: + /// - the pending redemptions value for the wallet is decreased + /// by the requested amount (minus treasury fee), + /// - the request is removed from pending redemptions mapping, + /// - the tokens taken from the redeemer on redemption request are + /// detained and passed to the redemption watchtower + /// (as Bank's balance) for further processing. + /// @param walletPubKeyHash 20-byte public key hash of the wallet. + /// @param redeemerOutputScript The redeemer's length-prefixed output + /// script (P2PKH, P2WPKH, P2SH or P2WSH). + /// @dev Requirements: + /// - The caller must be the redemption watchtower, + /// - The redemption request identified by `walletPubKeyHash` and + /// `redeemerOutputScript` must exist. + function notifyRedemptionVeto( + bytes20 walletPubKeyHash, + bytes calldata redeemerOutputScript + ) external { + // The caller is checked in the internal function. + self.notifyRedemptionVeto(walletPubKeyHash, redeemerOutputScript); + } } diff --git a/solidity/contracts/bridge/Redemption.sol b/solidity/contracts/bridge/Redemption.sol index 1d52aac9c..bcba9ff91 100644 --- a/solidity/contracts/bridge/Redemption.sol +++ b/solidity/contracts/bridge/Redemption.sol @@ -402,6 +402,8 @@ library Redemption { bytes memory redeemerOutputScript, uint64 amount ) internal { + // TODO: Validate the request against the RedemptionWatchtower. + Wallets.Wallet storage wallet = self.registeredWallets[ walletPubKeyHash ]; @@ -1075,4 +1077,64 @@ library Redemption { } return key; } + + /// @notice Notifies that a redemption request was vetoed in the watchtower. + /// This function is responsible for adjusting the Bridge's state + /// accordingly. + /// The results of calling this function: + /// - the pending redemptions value for the wallet is decreased + /// by the requested amount (minus treasury fee), + /// - the request is removed from pending redemptions mapping, + /// - the tokens taken from the redeemer on redemption request are + /// detained and passed to the redemption watchtower + /// (as Bank's balance) for further processing. + /// @param walletPubKeyHash 20-byte public key hash of the wallet. + /// @param redeemerOutputScript The redeemer's length-prefixed output + /// script (P2PKH, P2WPKH, P2SH or P2WSH). + /// @dev Requirements: + /// - The caller must be the redemption watchtower, + /// - The redemption request identified by `walletPubKeyHash` and + /// `redeemerOutputScript` must exist. + function notifyRedemptionVeto( + BridgeState.Storage storage self, + bytes20 walletPubKeyHash, + bytes calldata redeemerOutputScript + ) external { + require( + msg.sender == self.redemptionWatchtower, + "Caller is not the redemption watchtower" + ); + + uint256 redemptionKey = getRedemptionKey( + walletPubKeyHash, + redeemerOutputScript + ); + Redemption.RedemptionRequest storage redemption = self + .pendingRedemptions[redemptionKey]; + + // Should never happen, but just in case. + require( + redemption.requestedAt != 0, + "Redemption request does not exist" + ); + + // Update the wallet's pending redemptions value. This is the + // same logic as performed upon redemption request timeout. + // If we don't do this, the wallet will hold the reserve + // for a redemption request that will never be processed. + self.registeredWallets[walletPubKeyHash].pendingRedemptionsValue -= + redemption.requestedAmount - + redemption.treasuryFee; + + // Capture the amount that should be transferred to the + // redemption watchtower. + uint64 detainedAmount = redemption.requestedAmount; + + // Delete the redemption request from the pending redemptions + // mapping. This is important to avoid this redemption request + // to be processed by the wallet or reported as timed out. + delete self.pendingRedemptions[redemptionKey]; + + self.bank.transferBalance(self.redemptionWatchtower, detainedAmount); + } } diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index d5876d830..eed221cc0 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -18,6 +18,7 @@ pragma solidity 0.8.17; import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; import "./Bridge.sol"; +import "./Redemption.sol"; /// @title Redemption watchtower /// @notice This contract encapsulates the logic behind the redemption veto @@ -95,6 +96,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { /// veto request was finalized (i.e. moment of the last guardian /// objection that caused finalization). Value in seconds. uint32 public vetoFreezePeriod; + /// @notice The Bank contract. + Bank public bank; /// @notice Default delay applied to each redemption request. It is the time /// during which redemption guardians can raise the first objection. /// Wallets are not allowed to finalize the redemption request before @@ -122,11 +125,23 @@ contract RedemptionWatchtower is OwnableUpgradeable { event GuardianRemoved(address indexed guardian); + event ObjectionRaised( + uint256 indexed redemptionKey, + address indexed guardian + ); + + event VetoFinalized(uint256 indexed redemptionKey); + modifier onlyManager() { require(msg.sender == manager, "Caller is not watchtower manager"); _; } + modifier onlyGuardian() { + require(isGuardian[msg.sender], "Caller is not guardian"); + _; + } + /// @custom:oz-upgrades-unsafe-allow constructor constructor() { _disableInitializers(); @@ -136,6 +151,7 @@ contract RedemptionWatchtower is OwnableUpgradeable { __Ownable_init(); bridge = _bridge; + (bank, , , ) = _bridge.contractReferences(); watchtowerLifetime = 18 * 30 days; // 18 months vetoPenaltyFeeDivisor = 1; // 100% as initial penalty fee @@ -201,4 +217,135 @@ contract RedemptionWatchtower is OwnableUpgradeable { delete isGuardian[guardian]; emit GuardianRemoved(guardian); } + + /// @notice Raises an objection to a redemption request identified by the + /// key built as `keccak256(keccak256(redeemerOutputScript) | walletPubKeyHash)`. + /// Each redemption has a default delay period during which + /// the wallet is not allowed to process it and the guardians + /// can raise objections to. Each objection extends the delay + /// period by a certain amount of time. The third objection + /// vetoes the redemption request. This causes the redemption + /// request to be rejected and the redeemer to be penalized. + /// Specific consequences of a veto are as follows: + /// - The redemption amount is frozen for a certain period of time, + /// - Once the freeze period expires, the redeemer can claim the + /// frozen amount minus a penalty fee, + /// - The penalty fee is burned, + /// - The redeemer is banned from making future redemption requests. + /// @param walletPubKeyHash 20-byte public key hash of the wallet. + /// @param redeemerOutputScript The redeemer's length-prefixed output + /// script (P2PKH, P2WPKH, P2SH or P2WSH). + /// @dev Requirements: + /// - The caller must be a redemption guardian, + /// - The redemption request must exist (i.e. must be pending), + /// - The redemption request must not have been vetoed already, + /// - The guardian must not have already objected to the redemption request, + /// - The redemption request must be within the optimistic redemption + /// delay period. The only exception is when the redemption request + /// was created before the optimistic redemption mechanism + /// initialization timestamp. In this case, the redemption request + /// can be objected to without any time restrictions. + function raiseObjection( + bytes20 walletPubKeyHash, + bytes calldata redeemerOutputScript + ) external onlyGuardian { + uint256 redemptionKey = Redemption.getRedemptionKey( + walletPubKeyHash, + redeemerOutputScript + ); + + Redemption.RedemptionRequest memory redemption = bridge + .pendingRedemptions(redemptionKey); + + require( + redemption.requestedAt != 0, + "Redemption request does not exist" + ); + + VetoProposal storage veto = vetoProposals[redemptionKey]; + + uint8 requiredObjectionsCount = 3; + + require( + veto.objectionsCount < requiredObjectionsCount, + "Redemption request already vetoed" + ); + + uint256 objectionKey = uint256( + keccak256(abi.encodePacked(redemptionKey, msg.sender)) + ); + require(!objections[objectionKey], "Guardian already objected"); + + // Check if the given redemption request can be objected to: + // - Objections against a redemption request created AFTER the + // `watchtowerEnabledAt` timestamp can be raised only within + // a certain time frame defined by the redemption delay. + // - Objections against a redemption request created BEFORE the + // `watchtowerEnabledAt` timestamp can be raised without + // any time restrictions. + uint32 delay = _redemptionDelay(veto.objectionsCount); + if (redemption.requestedAt >= watchtowerEnabledAt) { + require( + /* solhint-disable-next-line not-rely-on-time */ + block.timestamp < redemption.requestedAt + delay, + "Redemption veto delay period expired" + ); + } + + objections[objectionKey] = true; + // Set the redeemer address in the veto request early to slightly + // reduce gas costs for the last guardian that must pay for the + // veto finalization. + veto.redeemer = redemption.redeemer; + veto.objectionsCount++; + + emit ObjectionRaised(redemptionKey, msg.sender); + + // If there are enough objections, finalize the veto. + if (veto.objectionsCount == requiredObjectionsCount) { + // Calculate the veto penalty fee that will be deducted from the + // final amount that the redeemer can claim after the freeze period. + uint64 penaltyFee = vetoPenaltyFeeDivisor > 0 + ? redemption.requestedAmount / vetoPenaltyFeeDivisor + : 0; + + // Set finalization fields in the veto request. + veto.claimableAmount = redemption.requestedAmount - penaltyFee; + /* solhint-disable-next-line not-rely-on-time */ + veto.finalizedAt = uint32(block.timestamp); + // Mark the redeemer as banned to prevent future redemption + // requests from that address. + isBanned[redemption.redeemer] = true; + + emit VetoFinalized(redemptionKey); + + // Notify the Bridge about the veto. As result of this call, + // this contract should receive the requested redemption amount + // (as Bank's balance) from the Bridge. + bridge.notifyRedemptionVeto(walletPubKeyHash, redeemerOutputScript); + // Burn the penalty fee but leave the claimable amount. The + // claimable amount will be returned to the redeemer after the + // freeze period. + bank.decreaseBalance(penaltyFee); + } + } + + /// @notice Returns the redemption delay for a given number of objections. + /// @param objectionsCount Number of objections. + /// @return delay Redemption delay. + function _redemptionDelay(uint8 objectionsCount) + internal + view + returns (uint32 delay) + { + if (objectionsCount == 0) { + delay = defaultDelay; + } else if (objectionsCount == 1) { + delay = levelOneDelay; + } else if (objectionsCount == 2) { + delay = levelTwoDelay; + } else { + revert("No delay for given objections count"); + } + } } diff --git a/solidity/contracts/bridge/WalletProposalValidator.sol b/solidity/contracts/bridge/WalletProposalValidator.sol index ab1d400e1..ae598e703 100644 --- a/solidity/contracts/bridge/WalletProposalValidator.sol +++ b/solidity/contracts/bridge/WalletProposalValidator.sol @@ -608,6 +608,8 @@ contract WalletProposalValidator { "Not a pending redemption request" ); + // TODO: Validate the request against the RedemptionWatchtower. + require( /* solhint-disable-next-line not-rely-on-time */ block.timestamp > From 6c5dc8dbaf10603f433ae3e7ad0a4b51a1d484b2 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 20 Feb 2024 12:53:20 +0100 Subject: [PATCH 05/23] Better observability for objections related to legacy redemptions Here we introduce the `VetoPeriodCheckOmitted` event that is emitted when a redemption objection omits the veto period check. Such a case occurs when the objection is related to a redemption created before the watchtower was enabled. --- solidity/contracts/bridge/RedemptionWatchtower.sol | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index eed221cc0..4f04377c0 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -130,6 +130,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { address indexed guardian ); + event VetoPeriodCheckOmitted(uint256 indexed redemptionKey); + event VetoFinalized(uint256 indexed redemptionKey); modifier onlyManager() { @@ -283,13 +285,16 @@ contract RedemptionWatchtower is OwnableUpgradeable { // - Objections against a redemption request created BEFORE the // `watchtowerEnabledAt` timestamp can be raised without // any time restrictions. - uint32 delay = _redemptionDelay(veto.objectionsCount); if (redemption.requestedAt >= watchtowerEnabledAt) { require( /* solhint-disable-next-line not-rely-on-time */ - block.timestamp < redemption.requestedAt + delay, + block.timestamp < + redemption.requestedAt + + _redemptionDelay(veto.objectionsCount), "Redemption veto delay period expired" ); + } else { + emit VetoPeriodCheckOmitted(redemptionKey); } objections[objectionKey] = true; From 68d5efd310cbbe890c172de7c510f1ba96da464d Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 20 Feb 2024 12:57:31 +0100 Subject: [PATCH 06/23] Expose a function allowing to update watchtower parameters --- .../contracts/bridge/RedemptionWatchtower.sol | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index 4f04377c0..4a1517d70 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -134,6 +134,15 @@ contract RedemptionWatchtower is OwnableUpgradeable { event VetoFinalized(uint256 indexed redemptionKey); + event WatchtowerParametersUpdated( + uint32 watchtowerLifetime, + uint64 vetoPenaltyFeeDivisor, + uint32 vetoFreezePeriod, + uint32 defaultDelay, + uint32 levelOneDelay, + uint32 levelTwoDelay + ); + modifier onlyManager() { require(msg.sender == manager, "Caller is not watchtower manager"); _; @@ -353,4 +362,67 @@ contract RedemptionWatchtower is OwnableUpgradeable { revert("No delay for given objections count"); } } + + /// @notice Updates the watchtower parameters. + /// @param _watchtowerLifetime Duration of the watchtower lifetime in seconds. + /// @param _vetoPenaltyFeeDivisor Divisor used to compute the redemption veto + /// penalty fee deducted upon veto finalization. + /// @param _vetoFreezePeriod Time of the redemption veto freeze period. + /// @param _defaultDelay Default delay applied to each redemption request. + /// @param _levelOneDelay Delay applied to redemption requests a single guardian + /// raised an objection to. + /// @param _levelTwoDelay Delay applied to redemption requests two guardians + /// raised an objection to. + /// @dev Requirements: + /// - The caller must be the watchtower manager, + /// - The new watchtower lifetime must not be lesser than the current one, + /// - The new redemption veto penalty fee divisor must be in range [0%, 5%], + /// - The new redemption level-two delay must not be lesser than level-one delay, + /// - The new redemption level-one delay must not be lesser than default delay. + function updateWatchtowerParameters( + uint32 _watchtowerLifetime, + uint64 _vetoPenaltyFeeDivisor, + uint32 _vetoFreezePeriod, + uint32 _defaultDelay, + uint32 _levelOneDelay, + uint32 _levelTwoDelay + ) external onlyManager { + require( + _watchtowerLifetime >= watchtowerLifetime, + "New lifetime must not be lesser than current one" + ); + + // Enforce the 5% hard cap. + require( + _vetoPenaltyFeeDivisor >= 20 || _vetoPenaltyFeeDivisor == 0, + "Redemption veto penalty fee must be in range [0%, 5%]" + ); + + // Enforce proper relationship between the delay levels. Use + // `>=` to allow for setting all delays to zero, if needed. + require( + _levelTwoDelay >= _levelOneDelay, + "Redemption level-two delay must not be lesser than level-one delay" + ); + require( + _levelOneDelay >= _defaultDelay, + "Redemption level-one delay must not be lesser than default delay" + ); + + watchtowerLifetime = _watchtowerLifetime; + vetoPenaltyFeeDivisor = _vetoPenaltyFeeDivisor; + vetoFreezePeriod = _vetoFreezePeriod; + defaultDelay = _defaultDelay; + levelOneDelay = _levelOneDelay; + levelTwoDelay = _levelTwoDelay; + + emit WatchtowerParametersUpdated( + _watchtowerLifetime, + _vetoPenaltyFeeDivisor, + _vetoFreezePeriod, + _defaultDelay, + _levelOneDelay, + _levelTwoDelay + ); + } } From 6aad28a109d63db27ef8c92aa59ff52939c19268 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 20 Feb 2024 13:22:48 +0100 Subject: [PATCH 07/23] Change order of checks in `raiseObjection` function Perform proposal and guardian checks first to fail fast and avoid burning gas in case a guardian try to object again by mistake. This flow is also more natural as the redemption request is checked closer to the point of usage. --- .../contracts/bridge/RedemptionWatchtower.sol | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index 4a1517d70..15adab777 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -248,9 +248,9 @@ contract RedemptionWatchtower is OwnableUpgradeable { /// script (P2PKH, P2WPKH, P2SH or P2WSH). /// @dev Requirements: /// - The caller must be a redemption guardian, - /// - The redemption request must exist (i.e. must be pending), /// - The redemption request must not have been vetoed already, /// - The guardian must not have already objected to the redemption request, + /// - The redemption request must exist (i.e. must be pending), /// - The redemption request must be within the optimistic redemption /// delay period. The only exception is when the redemption request /// was created before the optimistic redemption mechanism @@ -265,14 +265,6 @@ contract RedemptionWatchtower is OwnableUpgradeable { redeemerOutputScript ); - Redemption.RedemptionRequest memory redemption = bridge - .pendingRedemptions(redemptionKey); - - require( - redemption.requestedAt != 0, - "Redemption request does not exist" - ); - VetoProposal storage veto = vetoProposals[redemptionKey]; uint8 requiredObjectionsCount = 3; @@ -287,6 +279,14 @@ contract RedemptionWatchtower is OwnableUpgradeable { ); require(!objections[objectionKey], "Guardian already objected"); + Redemption.RedemptionRequest memory redemption = bridge + .pendingRedemptions(redemptionKey); + + require( + redemption.requestedAt != 0, + "Redemption request does not exist" + ); + // Check if the given redemption request can be objected to: // - Objections against a redemption request created AFTER the // `watchtowerEnabledAt` timestamp can be raised only within From 45d3afb5e5393ec9d335c7f66520195b33a57fc3 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 20 Feb 2024 13:31:00 +0100 Subject: [PATCH 08/23] Unit tests for `raiseObjection` function --- .../test/bridge/RedemptionWatchtower.test.ts | 1132 ++++++++++++++++- 1 file changed, 1129 insertions(+), 3 deletions(-) diff --git a/solidity/test/bridge/RedemptionWatchtower.test.ts b/solidity/test/bridge/RedemptionWatchtower.test.ts index 4d1d396cb..3349ed82c 100644 --- a/solidity/test/bridge/RedemptionWatchtower.test.ts +++ b/solidity/test/bridge/RedemptionWatchtower.test.ts @@ -1,12 +1,24 @@ import { helpers, waffle, ethers } from "hardhat" import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers" import { expect } from "chai" -import { ContractTransaction } from "ethers" -import type { Bridge, BridgeStub, RedemptionWatchtower } from "../../typechain" +import { BigNumber, BigNumberish, BytesLike, ContractTransaction } from "ethers" +import type { + Bank, + BankStub, + Bridge, + BridgeGovernance, + BridgeStub, + RedemptionWatchtower, +} from "../../typechain" import bridgeFixture from "../fixtures/bridge" +import { + RedemptionTestData, + SinglePendingRequestedRedemption, +} from "../data/redemption" +const { impersonateAccount } = helpers.account const { createSnapshot, restoreSnapshot } = helpers.snapshot -const { lastBlockTime } = helpers.time +const { lastBlockTime, increaseTime } = helpers.time describe("RedemptionWatchtower", () => { let governance: SignerWithAddress @@ -14,7 +26,10 @@ describe("RedemptionWatchtower", () => { let redemptionWatchtowerManager: SignerWithAddress let guardians: SignerWithAddress[] + let bridgeGovernance: BridgeGovernance let bridge: Bridge & BridgeStub + let bank: Bank & BankStub + let redemptionWatchtower: RedemptionWatchtower before(async () => { @@ -24,10 +39,16 @@ describe("RedemptionWatchtower", () => { thirdParty, redemptionWatchtowerManager, guardians, + bridgeGovernance, bridge, + bank, redemptionWatchtower, } = await waffle.loadFixture(bridgeFixture)) + await bridgeGovernance + .connect(governance) + .setRedemptionWatchtower(redemptionWatchtower.address) + // Make sure test actors are correctly set up. const actors = [ governance, @@ -279,4 +300,1109 @@ describe("RedemptionWatchtower", () => { }) }) }) + + describe("raiseObjection", () => { + let legacyRedemption: RedemptionData + + // Create a redemption request before enabling the watchtower. + // Such a request is needed for the scenario that checks if pre-watchtower + // requests can be vetoed indefinitely. As SinglePendingRequestedRedemption + // is used for post-watchtower requests as well, we need to modify the + // redeemerOutputScript to avoid a collision and obtain different redemption + // keys. + const createLegacyRedemption = async () => { + const data: RedemptionTestData = JSON.parse( + JSON.stringify(SinglePendingRequestedRedemption) + ) + data.redemptionRequests[0].redeemerOutputScript = + "0x1976a9142cd680318747b720d67bf4246eb7403b476adb3488ac" + const redemptions = await createRedemptionRequests(data) + // eslint-disable-next-line prefer-destructuring + return redemptions[0] + } + + before(async () => { + await createSnapshot() + + legacyRedemption = await createLegacyRedemption() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + + // Update the default penalty fee from 100% to 5% to test the penalty fee + // calculation. + await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + await redemptionWatchtower.watchtowerLifetime(), + 20, + await redemptionWatchtower.vetoFreezePeriod(), + await redemptionWatchtower.defaultDelay(), + await redemptionWatchtower.levelOneDelay(), + await redemptionWatchtower.levelTwoDelay() + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when called not by a guardian", () => { + it("should revert", async () => { + // No need to create the redemption request. The caller check is + // performed before the redemption request existence check. + const { pubKeyHash } = SinglePendingRequestedRedemption.wallet + const { redeemerOutputScript } = + SinglePendingRequestedRedemption.redemptionRequests[0] + + await expect( + redemptionWatchtower + .connect(thirdParty) + .raiseObjection(pubKeyHash, redeemerOutputScript) + ).to.be.revertedWith("Caller is not guardian") + }) + }) + + context("when called by a guardian", () => { + context("when redemption request is already vetoed", () => { + let redemption: RedemptionData + + before(async () => { + await createSnapshot() + + const redemptions = await createRedemptionRequests( + SinglePendingRequestedRedemption + ) + // eslint-disable-next-line prefer-destructuring + redemption = redemptions[0] + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Raise the second objection. + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Raise the third objection. + await redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Add the 4th guardian that will attempt to raise a redundant + // objection. + await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .addGuardian(thirdParty.address) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(thirdParty) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + ).to.be.revertedWith("Redemption request already vetoed") + }) + }) + + context("when redemption request is not vetoed yet", () => { + context("when guardian already objected", () => { + let redemption: RedemptionData + + before(async () => { + await createSnapshot() + + const redemptions = await createRedemptionRequests( + SinglePendingRequestedRedemption + ) + // eslint-disable-next-line prefer-destructuring + redemption = redemptions[0] + + // Raise the objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + ).to.be.revertedWith("Guardian already objected") + }) + }) + + context("when guardian did not object yet", () => { + context("when redemption request does not exist", () => { + it("should revert", async () => { + const { pubKeyHash } = SinglePendingRequestedRedemption.wallet + const { redeemerOutputScript } = + SinglePendingRequestedRedemption.redemptionRequests[0] + + await expect( + redemptionWatchtower + .connect(guardians[0]) + .raiseObjection(pubKeyHash, redeemerOutputScript) + ).to.be.revertedWith("Redemption request does not exist") + }) + }) + + context("when redemption request exists", () => { + context( + "when delay period expired and request was created after mechanism initialization", + () => { + let redemption: RedemptionData + let defaultDelay: number + let levelOneDelay: number + let levelTwoDelay: number + + before(async () => { + await createSnapshot() + + defaultDelay = await redemptionWatchtower.defaultDelay() + levelOneDelay = await redemptionWatchtower.levelOneDelay() + levelTwoDelay = await redemptionWatchtower.levelTwoDelay() + + const redemptions = await createRedemptionRequests( + SinglePendingRequestedRedemption + ) + // eslint-disable-next-line prefer-destructuring + redemption = redemptions[0] + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the raised objection is the first one", () => { + before(async () => { + await createSnapshot() + + // Set time to the first possible moment the first objection + // can no longer be raised. We need to subtract 1 seconds + // to make sure the `raiseObjection` transaction + // is mined exactly at the timestamp the delay expires. + const delayExpiresAt = redemption.requestedAt + defaultDelay + await increaseTime( + delayExpiresAt - (await lastBlockTime()) - 1 + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + ).to.be.revertedWith("Redemption veto delay period expired") + }) + }) + + context("when the raised objection is the second one", () => { + before(async () => { + await createSnapshot() + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Set time to the first possible moment the second objection + // can no longer be raised. We need to subtract 1 seconds + // to make sure the `raiseObjection` transaction + // is mined exactly at the timestamp the delay expires. + const delayExpiresAt = + redemption.requestedAt + levelOneDelay + await increaseTime( + delayExpiresAt - (await lastBlockTime()) - 1 + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + ).to.be.revertedWith("Redemption veto delay period expired") + }) + }) + + context("when the raised objection is the third one", () => { + before(async () => { + await createSnapshot() + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Raise the second objection. + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Set time to the first possible moment the third objection + // can no longer be raised. We need to subtract 1 seconds + // to make sure the `raiseObjection` transaction + // is mined exactly at the timestamp the delay expires. + const delayExpiresAt = + redemption.requestedAt + levelTwoDelay + await increaseTime( + delayExpiresAt - (await lastBlockTime()) - 1 + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + ).to.be.revertedWith("Redemption veto delay period expired") + }) + }) + } + ) + + context( + "when delay period expired but request was created before mechanism initialization", + () => { + before(async () => { + await createSnapshot() + + // Use the legacy redemption created before the watchtower was enabled. + // Jump to a moment when the delay period expired for sure + // (use the maximum level-two delay). + const levelTwoDelay = + await redemptionWatchtower.levelTwoDelay() + const delayExpiresAt = + legacyRedemption.requestedAt + levelTwoDelay + await increaseTime(delayExpiresAt - (await lastBlockTime())) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the raised objection is the first one", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + // Raise the first objection. + tx = await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + legacyRedemption.walletPublicKeyHash, + legacyRedemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should emit VetoPeriodCheckOmitted event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "VetoPeriodCheckOmitted") + .withArgs(legacyRedemption.redemptionKey) + }) + + it("should store the objection key", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.objections( + buildObjectionKey( + legacyRedemption.redemptionKey, + guardians[0].address + ) + ) + ).to.be.true + }) + + it("should update veto state properly", async () => { + expect( + await redemptionWatchtower.vetoProposals( + legacyRedemption.redemptionKey + ) + ).to.be.eql([ + legacyRedemption.redeemer, + BigNumber.from(0), + 0, + 1, + ]) + }) + + it("should emit ObjectionRaised event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "ObjectionRaised") + .withArgs( + legacyRedemption.redemptionKey, + guardians[0].address + ) + }) + }) + + context("when the raised objection is the second one", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + legacyRedemption.walletPublicKeyHash, + legacyRedemption.redeemerOutputScript + ) + + // Raise the second objection. + tx = await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + legacyRedemption.walletPublicKeyHash, + legacyRedemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should emit VetoPeriodCheckOmitted event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "VetoPeriodCheckOmitted") + .withArgs(legacyRedemption.redemptionKey) + }) + + it("should store the objection key", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.objections( + buildObjectionKey( + legacyRedemption.redemptionKey, + guardians[1].address + ) + ) + ).to.be.true + }) + + it("should update veto state properly", async () => { + expect( + await redemptionWatchtower.vetoProposals( + legacyRedemption.redemptionKey + ) + ).to.be.eql([ + legacyRedemption.redeemer, + BigNumber.from(0), + 0, + 2, + ]) + }) + + it("should emit ObjectionRaised event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "ObjectionRaised") + .withArgs( + legacyRedemption.redemptionKey, + guardians[1].address + ) + }) + }) + + context("when the raised objection is the third one", () => { + let tx: ContractTransaction + let initialWalletPendingRedemptionsValue: BigNumber + let initialBridgeBalance: BigNumber + let initialWatchtowerBalance: BigNumber + + before(async () => { + await createSnapshot() + + initialWalletPendingRedemptionsValue = ( + await bridge.wallets(legacyRedemption.walletPublicKeyHash) + ).pendingRedemptionsValue + + initialBridgeBalance = await bank.balanceOf(bridge.address) + + initialWatchtowerBalance = await bank.balanceOf( + redemptionWatchtower.address + ) + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + legacyRedemption.walletPublicKeyHash, + legacyRedemption.redeemerOutputScript + ) + // Raise the second objection. + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + legacyRedemption.walletPublicKeyHash, + legacyRedemption.redeemerOutputScript + ) + + // Raise the third objection. + tx = await redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + legacyRedemption.walletPublicKeyHash, + legacyRedemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should emit VetoPeriodCheckOmitted event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "VetoPeriodCheckOmitted") + .withArgs(legacyRedemption.redemptionKey) + }) + + it("should store the objection key", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.objections( + buildObjectionKey( + legacyRedemption.redemptionKey, + guardians[2].address + ) + ) + ).to.be.true + }) + + it("should update veto state properly", async () => { + // Penalty fee is 5% of the redemption amount. + const penaltyFee = legacyRedemption.amount.mul(5).div(100) + // The claimable amount left on the watchtower should + // be equal to the redemption amount minus the penalty fee. + const claimableAmount = + legacyRedemption.amount.sub(penaltyFee) + + expect( + await redemptionWatchtower.vetoProposals( + legacyRedemption.redemptionKey + ) + ).to.be.eql([ + legacyRedemption.redeemer, + claimableAmount, + // Finalization time is equal to the last block time. + await lastBlockTime(), + 3, + ]) + }) + + it("should emit ObjectionRaised event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "ObjectionRaised") + .withArgs( + legacyRedemption.redemptionKey, + guardians[2].address + ) + }) + + it("should mark the redeemer as banned", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.isBanned( + legacyRedemption.redeemer + ) + ).to.be.true + }) + + it("should emit VetoFinalized event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "VetoFinalized") + .withArgs(legacyRedemption.redemptionKey) + }) + + it("should decrease wallet's pending redemptions value in the Bridge", async () => { + const currentWalletPendingRedemptionsValue = ( + await bridge.wallets(legacyRedemption.walletPublicKeyHash) + ).pendingRedemptionsValue + + const difference = initialWalletPendingRedemptionsValue.sub( + currentWalletPendingRedemptionsValue + ) + + expect(difference).to.be.equal( + legacyRedemption.amount.sub(legacyRedemption.treasuryFee) + ) + }) + + it("should remove pending redemption in the Bridge", async () => { + const { requestedAt } = await bridge.pendingRedemptions( + legacyRedemption.redemptionKey + ) + + expect(requestedAt).to.be.equal(0) + }) + + it("should transfer the redemption amount from the Bridge", async () => { + const currentBridgeBalance = await bank.balanceOf( + bridge.address + ) + + const difference = + initialBridgeBalance.sub(currentBridgeBalance) + + // The entire amount should be transferred to the watchtower. + expect(difference).to.be.equal(legacyRedemption.amount) + + // Double-check the right event was emitted. + await expect(tx) + .to.emit(bank, "BalanceTransferred") + .withArgs( + bridge.address, + redemptionWatchtower.address, + legacyRedemption.amount + ) + }) + + it("should leave a proper claimable amount and burn the penalty fee", async () => { + const currentWatchtowerBalance = await bank.balanceOf( + redemptionWatchtower.address + ) + + const difference = currentWatchtowerBalance.sub( + initialWatchtowerBalance + ) + + // Penalty fee is 5% of the redemption amount. + const penaltyFee = legacyRedemption.amount.mul(5).div(100) + + // The claimable amount left on the watchtower should + // be equal to the redemption amount minus the penalty fee. + expect(difference).to.be.equal( + legacyRedemption.amount.sub(penaltyFee) + ) + + // Make sure the penalty fee was burned. + await expect(tx) + .to.emit(bank, "BalanceDecreased") + .withArgs(redemptionWatchtower.address, penaltyFee) + }) + }) + } + ) + + context("when delay period did not expire yet", () => { + let redemption: RedemptionData + let defaultDelay: number + let levelOneDelay: number + let levelTwoDelay: number + + before(async () => { + await createSnapshot() + + defaultDelay = await redemptionWatchtower.defaultDelay() + levelOneDelay = await redemptionWatchtower.levelOneDelay() + levelTwoDelay = await redemptionWatchtower.levelTwoDelay() + + const redemptions = await createRedemptionRequests( + SinglePendingRequestedRedemption + ) + // eslint-disable-next-line prefer-destructuring + redemption = redemptions[0] + }) + + after(async () => { + await restoreSnapshot() + }) + + context( + "when the raised objection is the first one", + async () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + // Set time to the latest possible moment the first + // objection can be raised. We need to subtract 2 seconds + // to make sure the `raiseObjection` transaction + // is mined 1 second before the delay expires. + const delayExpiresAt = redemption.requestedAt + defaultDelay + await increaseTime( + delayExpiresAt - (await lastBlockTime()) - 2 + ) + + // Raise the first objection. + tx = await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should not emit VetoPeriodCheckOmitted event", async () => { + await expect(tx).to.not.emit( + redemptionWatchtower, + "VetoPeriodCheckOmitted" + ) + }) + + it("should store the objection key", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.objections( + buildObjectionKey( + redemption.redemptionKey, + guardians[0].address + ) + ) + ).to.be.true + }) + + it("should update veto state properly", async () => { + expect( + await redemptionWatchtower.vetoProposals( + redemption.redemptionKey + ) + ).to.be.eql([redemption.redeemer, BigNumber.from(0), 0, 1]) + }) + + it("should emit ObjectionRaised event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "ObjectionRaised") + .withArgs(redemption.redemptionKey, guardians[0].address) + }) + } + ) + + context("when the raised objection is the second one", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Set time to the latest possible moment the second + // objection can be raised. We need to subtract 2 seconds + // to make sure the `raiseObjection` transaction + // is mined 1 second before the delay expires. + const delayExpiresAt = redemption.requestedAt + levelOneDelay + await increaseTime( + delayExpiresAt - (await lastBlockTime()) - 2 + ) + + // Raise the second objection. + tx = await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should not emit VetoPeriodCheckOmitted event", async () => { + await expect(tx).to.not.emit( + redemptionWatchtower, + "VetoPeriodCheckOmitted" + ) + }) + + it("should store the objection key", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.objections( + buildObjectionKey( + redemption.redemptionKey, + guardians[1].address + ) + ) + ).to.be.true + }) + + it("should update veto state properly", async () => { + expect( + await redemptionWatchtower.vetoProposals( + redemption.redemptionKey + ) + ).to.be.eql([redemption.redeemer, BigNumber.from(0), 0, 2]) + }) + + it("should emit ObjectionRaised event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "ObjectionRaised") + .withArgs(redemption.redemptionKey, guardians[1].address) + }) + }) + + context("when the raised objection is the third one", () => { + let tx: ContractTransaction + let initialWalletPendingRedemptionsValue: BigNumber + let initialBridgeBalance: BigNumber + let initialWatchtowerBalance: BigNumber + + before(async () => { + await createSnapshot() + + initialWalletPendingRedemptionsValue = ( + await bridge.wallets(redemption.walletPublicKeyHash) + ).pendingRedemptionsValue + + initialBridgeBalance = await bank.balanceOf(bridge.address) + + initialWatchtowerBalance = await bank.balanceOf( + redemptionWatchtower.address + ) + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + // Raise the second objection. + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Set time to the latest possible moment the third + // objection can be raised. We need to subtract 2 seconds + // to make sure the `raiseObjection` transaction + // is mined 1 second before the delay expires. + const delayExpiresAt = redemption.requestedAt + levelTwoDelay + await increaseTime( + delayExpiresAt - (await lastBlockTime()) - 2 + ) + + // Raise the third objection. + tx = await redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should not emit VetoPeriodCheckOmitted event", async () => { + await expect(tx).to.not.emit( + redemptionWatchtower, + "VetoPeriodCheckOmitted" + ) + }) + + it("should store the objection key", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.objections( + buildObjectionKey( + redemption.redemptionKey, + guardians[2].address + ) + ) + ).to.be.true + }) + + it("should update veto state properly", async () => { + // Penalty fee is 5% of the redemption amount. + const penaltyFee = redemption.amount.mul(5).div(100) + // The claimable amount left on the watchtower should + // be equal to the redemption amount minus the penalty fee. + const claimableAmount = redemption.amount.sub(penaltyFee) + + expect( + await redemptionWatchtower.vetoProposals( + redemption.redemptionKey + ) + ).to.be.eql([ + redemption.redeemer, + claimableAmount, + // Finalization time is equal to the last block time. + await lastBlockTime(), + 3, + ]) + }) + + it("should emit ObjectionRaised event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "ObjectionRaised") + .withArgs(redemption.redemptionKey, guardians[2].address) + }) + + it("should mark the redeemer as banned", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect( + await redemptionWatchtower.isBanned(redemption.redeemer) + ).to.be.true + }) + + it("should emit VetoFinalized event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "VetoFinalized") + .withArgs(redemption.redemptionKey) + }) + + it("should decrease wallet's pending redemptions value in the Bridge", async () => { + const currentWalletPendingRedemptionsValue = ( + await bridge.wallets(redemption.walletPublicKeyHash) + ).pendingRedemptionsValue + + const difference = initialWalletPendingRedemptionsValue.sub( + currentWalletPendingRedemptionsValue + ) + + expect(difference).to.be.equal( + redemption.amount.sub(redemption.treasuryFee) + ) + }) + + it("should remove pending redemption in the Bridge", async () => { + const { requestedAt } = await bridge.pendingRedemptions( + redemption.redemptionKey + ) + + expect(requestedAt).to.be.equal(0) + }) + + it("should transfer the redemption amount from the Bridge", async () => { + const currentBridgeBalance = await bank.balanceOf( + bridge.address + ) + + const difference = + initialBridgeBalance.sub(currentBridgeBalance) + + // The entire amount should be transferred to the watchtower. + expect(difference).to.be.equal(redemption.amount) + + // Double-check the right event was emitted. + await expect(tx) + .to.emit(bank, "BalanceTransferred") + .withArgs( + bridge.address, + redemptionWatchtower.address, + redemption.amount + ) + }) + + it("should leave a proper claimable amount and burn the penalty fee", async () => { + const currentWatchtowerBalance = await bank.balanceOf( + redemptionWatchtower.address + ) + + const difference = currentWatchtowerBalance.sub( + initialWatchtowerBalance + ) + + // Penalty fee is 5% of the redemption amount. + const penaltyFee = redemption.amount.mul(5).div(100) + + // The claimable amount left on the watchtower should + // be equal to the redemption amount minus the penalty fee. + expect(difference).to.be.equal( + redemption.amount.sub(penaltyFee) + ) + + // Make sure the penalty fee was burned. + await expect(tx) + .to.emit(bank, "BalanceDecreased") + .withArgs(redemptionWatchtower.address, penaltyFee) + }) + }) + }) + }) + }) + }) + }) + }) + + type RedemptionData = { + redemptionKey: string + walletPublicKeyHash: string + redeemerOutputScript: string + redeemer: string + requestedAt: number + amount: BigNumber + treasuryFee: BigNumber + } + + async function createRedemptionRequests( + data: RedemptionTestData + ): Promise { + // Simulate the wallet is a registered one. + await bridge.setWallet(data.wallet.pubKeyHash, { + ecdsaWalletID: data.wallet.ecdsaWalletID, + mainUtxoHash: ethers.constants.HashZero, + pendingRedemptionsValue: data.wallet.pendingRedemptionsValue, + createdAt: await lastBlockTime(), + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: data.wallet.state, + movingFundsTargetWalletsCommitmentHash: ethers.constants.HashZero, + }) + + // Simulate the prepared main UTXO belongs to the wallet. + await bridge.setWalletMainUtxo(data.wallet.pubKeyHash, data.mainUtxo) + + const redemptions: RedemptionData[] = [] + + for (let i = 0; i < data.redemptionRequests.length; i++) { + const { redeemer, redeemerOutputScript, amount } = + data.redemptionRequests[i] + + /* eslint-disable no-await-in-loop */ + const redeemerSigner = await impersonateAccount(redeemer, { + from: governance, + value: 10, + }) + + await makeRedemptionAllowance(redeemerSigner, amount) + + await bridge + .connect(redeemerSigner) + .requestRedemption( + data.wallet.pubKeyHash, + data.mainUtxo, + redeemerOutputScript, + amount + ) + + const redemptionKey = buildRedemptionKey( + data.wallet.pubKeyHash, + redeemerOutputScript + ) + + const { requestedAt, treasuryFee } = await bridge.pendingRedemptions( + redemptionKey + ) + /* eslint-enable no-await-in-loop */ + + redemptions.push({ + redemptionKey, + walletPublicKeyHash: data.wallet.pubKeyHash.toString(), + redeemerOutputScript: redeemerOutputScript.toString(), + redeemer, + requestedAt, + amount: BigNumber.from(amount), + treasuryFee, + }) + } + + return redemptions + } + + async function makeRedemptionAllowance( + redeemer: SignerWithAddress, + amount: BigNumberish + ) { + // Simulate the redeemer has a Bank balance allowing to make the request. + await bank.setBalance(redeemer.address, amount) + // Redeemer must allow the Bridge to spent the requested amount. + await bank + .connect(redeemer) + .increaseBalanceAllowance(bridge.address, amount) + } + + function buildRedemptionKey( + walletPubKeyHash: BytesLike, + redeemerOutputScript: BytesLike + ): string { + return ethers.utils.solidityKeccak256( + ["bytes32", "bytes20"], + [ + ethers.utils.solidityKeccak256(["bytes"], [redeemerOutputScript]), + walletPubKeyHash, + ] + ) + } + + function buildObjectionKey(redemptionKey: string, guardian: string): string { + return ethers.utils.solidityKeccak256( + ["uint256", "address"], + [redemptionKey, guardian] + ) + } }) From 4556003b0c7abf8b05fae7b8b569589be475733b Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 20 Feb 2024 14:17:01 +0100 Subject: [PATCH 09/23] Unit tests for `updateWatchtowerParameters` function --- .../test/bridge/RedemptionWatchtower.test.ts | 279 ++++++++++++++++++ 1 file changed, 279 insertions(+) diff --git a/solidity/test/bridge/RedemptionWatchtower.test.ts b/solidity/test/bridge/RedemptionWatchtower.test.ts index 3349ed82c..e7f68e720 100644 --- a/solidity/test/bridge/RedemptionWatchtower.test.ts +++ b/solidity/test/bridge/RedemptionWatchtower.test.ts @@ -1298,6 +1298,285 @@ describe("RedemptionWatchtower", () => { }) }) + describe("updateWatchtowerParameters", () => { + let watchtowerLifetime: number + let vetoPenaltyFeeDivisor: number + let vetoFreezePeriod: number + let defaultDelay: number + let levelOneDelay: number + let levelTwoDelay: number + + before(async () => { + await createSnapshot() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + + watchtowerLifetime = await redemptionWatchtower.watchtowerLifetime() + vetoPenaltyFeeDivisor = 20 // Max value 5% + vetoFreezePeriod = await redemptionWatchtower.vetoFreezePeriod() + defaultDelay = await redemptionWatchtower.defaultDelay() + levelOneDelay = await redemptionWatchtower.levelOneDelay() + levelTwoDelay = await redemptionWatchtower.levelTwoDelay() + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when called not by the watchtower manager", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(thirdParty) + .updateWatchtowerParameters( + watchtowerLifetime, + vetoPenaltyFeeDivisor, + vetoFreezePeriod, + defaultDelay, + levelOneDelay, + levelTwoDelay + ) + ).to.be.revertedWith("Caller is not watchtower manager") + }) + }) + + context("when called by the watchtower manager", () => { + context("when new parameters are invalid", () => { + context("when the new lifetime is lesser than the current one", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + watchtowerLifetime - 1, // lesser than current value by 1 + vetoPenaltyFeeDivisor, + vetoFreezePeriod, + defaultDelay, + levelOneDelay, + levelTwoDelay + ) + ).to.be.revertedWith( + "New lifetime must not be lesser than current one" + ) + }) + }) + + context( + "when the new veto penalty fee is not in the proper range", + () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + watchtowerLifetime, + // Decrease the divisor by 1 to exceed the max value. + vetoPenaltyFeeDivisor - 1, + vetoFreezePeriod, + defaultDelay, + levelOneDelay, + levelTwoDelay + ) + ).to.be.revertedWith( + "Redemption veto penalty fee must be in range [0%, 5%]" + ) + }) + } + ) + + context("when level-two delay is lesser than level-one delay", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + watchtowerLifetime, + vetoPenaltyFeeDivisor, + vetoFreezePeriod, + defaultDelay, + levelOneDelay, + levelOneDelay - 1 + ) + ).to.be.revertedWith( + "Redemption level-two delay must not be lesser than level-one delay" + ) + }) + }) + + context("when level-one delay is lesser than default delay", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + watchtowerLifetime, + vetoPenaltyFeeDivisor, + vetoFreezePeriod, + defaultDelay, + defaultDelay - 1, + levelTwoDelay + ) + ).to.be.revertedWith( + "Redemption level-one delay must not be lesser than default delay" + ) + }) + }) + }) + + context("when all new parameters are valid", () => { + const testData: { + testName: string + newWatchtowerLifetime?: number + newVetoPenaltyFeeDivisor?: number + newVetoFreezePeriod?: number + newDefaultDelay?: number + newLevelOneDelay?: number + newLevelTwoDelay?: number + }[] = [ + { + testName: "when watchtower lifetime is increased", + newWatchtowerLifetime: 52_600_000, // 20 months + }, + { + testName: + "when veto penalty is changed to to the maximum value of 5%", + newVetoPenaltyFeeDivisor: 20, // 5% + }, + { + testName: + "when veto penalty is changed to to the middle of the range", + newVetoPenaltyFeeDivisor: 40, // 2.5% + }, + { + testName: "when veto penalty is changed to the minimum value of 0%", + newVetoPenaltyFeeDivisor: 0, // 0 % + }, + { + testName: "when veto freeze period is changed to a non-zero value", + newVetoFreezePeriod: 7200, // 2 hours + }, + { + testName: "when veto freeze period is changed to 0", + newVetoFreezePeriod: 0, + }, + { + testName: "when delays are changed to a non-zero value", + newDefaultDelay: 14400, // 4 hours + newLevelOneDelay: 57600, // 16 hours + newLevelTwoDelay: 115200, // 32 hours + }, + { + testName: "when delays are changed to 0", + newDefaultDelay: 0, + newLevelOneDelay: 0, + newLevelTwoDelay: 0, + }, + ] + + testData.forEach((test) => { + let newWatchtowerLifetime: number + let newVetoPenaltyFeeDivisor: number + let newVetoFreezePeriod: number + let newDefaultDelay: number + let newLevelOneDelay: number + let newLevelTwoDelay: number + + context(test.testName, async () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + const assignValue = (optionalValue, defaultValue) => + typeof optionalValue !== "undefined" + ? optionalValue + : defaultValue + + newWatchtowerLifetime = assignValue( + test.newWatchtowerLifetime, + watchtowerLifetime + ) + newVetoPenaltyFeeDivisor = assignValue( + test.newVetoPenaltyFeeDivisor, + vetoPenaltyFeeDivisor + ) + newVetoFreezePeriod = assignValue( + test.newVetoFreezePeriod, + vetoFreezePeriod + ) + newDefaultDelay = assignValue(test.newDefaultDelay, defaultDelay) + newLevelOneDelay = assignValue( + test.newLevelOneDelay, + levelOneDelay + ) + newLevelTwoDelay = assignValue( + test.newLevelTwoDelay, + levelTwoDelay + ) + + tx = await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + newWatchtowerLifetime, + newVetoPenaltyFeeDivisor, + newVetoFreezePeriod, + newDefaultDelay, + newLevelOneDelay, + newLevelTwoDelay + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should emit WatchtowerParametersUpdated event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "WatchtowerParametersUpdated") + .withArgs( + newWatchtowerLifetime, + newVetoPenaltyFeeDivisor, + newVetoFreezePeriod, + newDefaultDelay, + newLevelOneDelay, + newLevelTwoDelay + ) + }) + + it("should update the watchtower parameters", async () => { + expect( + await redemptionWatchtower.watchtowerLifetime() + ).to.be.equal(newWatchtowerLifetime) + + expect( + await redemptionWatchtower.vetoPenaltyFeeDivisor() + ).to.be.equal(newVetoPenaltyFeeDivisor) + + expect(await redemptionWatchtower.vetoFreezePeriod()).to.be.equal( + newVetoFreezePeriod + ) + + expect(await redemptionWatchtower.defaultDelay()).to.be.equal( + newDefaultDelay + ) + + expect(await redemptionWatchtower.levelOneDelay()).to.be.equal( + newLevelOneDelay + ) + + expect(await redemptionWatchtower.levelTwoDelay()).to.be.equal( + newLevelTwoDelay + ) + }) + }) + }) + }) + }) + }) + type RedemptionData = { redemptionKey: string walletPublicKeyHash: string From 912f96acd7cd1eaef79c3108633cae4d7bab7484 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 20 Feb 2024 14:46:50 +0100 Subject: [PATCH 10/23] Unit tests for `notifyRedemptionVeto` function --- .../test/bridge/Bridge.Redemption.test.ts | 163 ++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/solidity/test/bridge/Bridge.Redemption.test.ts b/solidity/test/bridge/Bridge.Redemption.test.ts index e0e453db6..bdf154304 100644 --- a/solidity/test/bridge/Bridge.Redemption.test.ts +++ b/solidity/test/bridge/Bridge.Redemption.test.ts @@ -40,6 +40,7 @@ import { } from "../data/redemption" import { constants, walletState } from "../fixtures" import bridgeFixture from "../fixtures/bridge" +import { RedemptionRequestStructOutput } from "../../typechain/Bridge" chai.use(smock.matchers) @@ -4605,6 +4606,168 @@ describe("Bridge - Redemption", () => { }) }) + describe("notifyRedemptionVeto", () => { + const data: RedemptionTestData = SinglePendingRequestedRedemption + const walletPublicKeyHash = data.wallet.pubKeyHash + const { redeemerOutputScript } = data.redemptionRequests[0] + + context("when the caller is not the redemption watchtower", () => { + it("should revert", async () => { + await expect( + bridge + .connect(thirdParty) + .notifyRedemptionVeto(walletPublicKeyHash, redeemerOutputScript) + ).to.be.revertedWith("Caller is not the redemption watchtower") + }) + }) + + context("when the caller is the redemption watchtower", () => { + let watchtower: SignerWithAddress + + before(async () => { + await createSnapshot() + + watchtower = thirdParty + + await bridgeGovernance + .connect(governance) + .setRedemptionWatchtower(watchtower.address) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the redemption does not exist", () => { + it("should revert", async () => { + await expect( + bridge + .connect(watchtower) + .notifyRedemptionVeto(walletPublicKeyHash, redeemerOutputScript) + ).to.be.revertedWith("Redemption request does not exist") + }) + }) + + context("when the redemption exists", () => { + let tx: ContractTransaction + + let redemptionKey: string + let redemption: RedemptionRequestStructOutput + let initialWalletPendingRedemptionsValue: BigNumber + let initialBridgeBalance: BigNumber + let initialWatchtowerBalance: BigNumber + + before(async () => { + await createSnapshot() + + await bridge.setWallet(walletPublicKeyHash, { + ecdsaWalletID: data.wallet.ecdsaWalletID, + mainUtxoHash: ethers.constants.HashZero, + pendingRedemptionsValue: data.wallet.pendingRedemptionsValue, + createdAt: await lastBlockTime(), + movingFundsRequestedAt: 0, + closingStartedAt: 0, + pendingMovedFundsSweepRequestsCount: 0, + state: walletState.Live, + movingFundsTargetWalletsCommitmentHash: ethers.constants.HashZero, + }) + await bridge.setWalletMainUtxo(walletPublicKeyHash, data.mainUtxo) + await bridge.setActiveWallet(walletPublicKeyHash) + + const redeemerSigner = await impersonateAccount( + data.redemptionRequests[0].redeemer, + { + from: governance, + value: 10, + } + ) + + await makeRedemptionAllowance( + redeemerSigner, + data.redemptionRequests[0].amount + ) + + await bridge + .connect(redeemerSigner) + .requestRedemption( + walletPublicKeyHash, + data.mainUtxo, + redeemerOutputScript, + data.redemptionRequests[0].amount + ) + + redemptionKey = buildRedemptionKey( + walletPublicKeyHash, + redeemerOutputScript + ) + redemption = await bridge.pendingRedemptions(redemptionKey) + + initialWalletPendingRedemptionsValue = ( + await bridge.wallets(walletPublicKeyHash) + ).pendingRedemptionsValue + + initialBridgeBalance = await bank.balanceOf(bridge.address) + initialWatchtowerBalance = await bank.balanceOf(watchtower.address) + + tx = await bridge + .connect(watchtower) + .notifyRedemptionVeto(walletPublicKeyHash, redeemerOutputScript) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should update the wallet's pending redemptions value", async () => { + const currentWalletPendingRedemptionsValue = ( + await bridge.wallets(walletPublicKeyHash) + ).pendingRedemptionsValue + + const difference = initialWalletPendingRedemptionsValue.sub( + currentWalletPendingRedemptionsValue + ) + + expect(difference).to.be.equal( + redemption.requestedAmount.sub(redemption.treasuryFee) + ) + }) + + it("should remove the request from the pending redemptions", async () => { + const request = await bridge.pendingRedemptions(redemptionKey) + + expect(request.requestedAt).to.be.equal(0) + }) + + it("should transfer the requested amount of tokens to the watchtower", async () => { + const currentBridgeBalance = await bank.balanceOf(bridge.address) + const currentWatchtowerBalance = await bank.balanceOf( + watchtower.address + ) + + // Bridge has a balance decrease. + const bridgeDifference = + initialBridgeBalance.sub(currentBridgeBalance) + // Watchtower has a balance increase. + const watchtowerDifference = currentWatchtowerBalance.sub( + initialWatchtowerBalance + ) + + expect(bridgeDifference).to.be.equal(redemption.requestedAmount) + expect(watchtowerDifference).to.be.equal(redemption.requestedAmount) + + // Double-check the right event was emitted. + await expect(tx) + .to.emit(bank, "BalanceTransferred") + .withArgs( + bridge.address, + watchtower.address, + redemption.requestedAmount + ) + }) + }) + }) + }) + interface RedemptionScenarioOutcome { tx: ContractTransaction bridgeBalance: RedemptionBalanceChange From c57f4f1880f4f198552d25a729580dbc0de8f7bd Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 20 Feb 2024 17:16:56 +0100 Subject: [PATCH 11/23] 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 | 1196 +++++++++-------- .../test/bridge/RedemptionWatchtower.test.ts | 140 ++ 4 files changed, 882 insertions(+), 529 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..7dee800d6 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 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 + 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() + + 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() - testData.forEach((test) => { - context(test.testName, () => { + await restoreSnapshot() + }) + + 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 }) - }) - }) + } + ) }) }) @@ -4622,12 +4750,22 @@ describe("Bridge - Redemption", () => { }) context("when the caller is the redemption watchtower", () => { - let watchtower: SignerWithAddress + let watchtower: FakeContract + let watchtowerSigner: SignerWithAddress before(async () => { await createSnapshot() - watchtower = thirdParty + watchtower = await smock.fake( + "IRedemptionWatchtower" + ) + + watchtower.isSafeRedemption.returns(true) + + watchtowerSigner = await impersonateAccount(watchtower.address, { + from: governance, + value: 10, + }) await bridgeGovernance .connect(governance) @@ -4635,6 +4773,8 @@ describe("Bridge - Redemption", () => { }) after(async () => { + watchtower.isSafeRedemption.reset() + await restoreSnapshot() }) @@ -4642,7 +4782,7 @@ describe("Bridge - Redemption", () => { it("should revert", async () => { await expect( bridge - .connect(watchtower) + .connect(watchtowerSigner) .notifyRedemptionVeto(walletPublicKeyHash, redeemerOutputScript) ).to.be.revertedWith("Redemption request does not exist") }) @@ -4710,7 +4850,7 @@ describe("Bridge - Redemption", () => { initialWatchtowerBalance = await bank.balanceOf(watchtower.address) tx = await bridge - .connect(watchtower) + .connect(watchtowerSigner) .notifyRedemptionVeto(walletPublicKeyHash, redeemerOutputScript) }) 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 From 3ab6ee5c57e4124df9cca6ff6fe1ece7bd4d8b0b Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 21 Feb 2024 11:03:08 +0100 Subject: [PATCH 12/23] Address Slither warning about variable shadowing --- solidity/contracts/bridge/Bridge.sol | 2 +- solidity/test/bridge/Bridge.Parameters.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/solidity/contracts/bridge/Bridge.sol b/solidity/contracts/bridge/Bridge.sol index 9f85e31c0..d54aeb350 100644 --- a/solidity/contracts/bridge/Bridge.sol +++ b/solidity/contracts/bridge/Bridge.sol @@ -1961,7 +1961,7 @@ contract Bridge is } /// @return Address of the redemption watchtower. - function redemptionWatchtower() external view returns (address) { + function getRedemptionWatchtower() external view returns (address) { return self.redemptionWatchtower; } diff --git a/solidity/test/bridge/Bridge.Parameters.test.ts b/solidity/test/bridge/Bridge.Parameters.test.ts index 9476303de..d848fd77f 100644 --- a/solidity/test/bridge/Bridge.Parameters.test.ts +++ b/solidity/test/bridge/Bridge.Parameters.test.ts @@ -1899,7 +1899,7 @@ describe("Bridge - Parameters", () => { }) it("should set the watchtower address", async () => { - expect(await bridge.redemptionWatchtower()).to.equal(watchtower) + expect(await bridge.getRedemptionWatchtower()).to.equal(watchtower) }) it("should emit RedemptionWatchtowerSet event", async () => { From a62b2e9b7af316fc21dc6d8c307d8262fd98d394 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 21 Feb 2024 13:21:30 +0100 Subject: [PATCH 13/23] Respect the watchtower's redemption delay within `WalletProposalValidator` Wallets must respect the new veto delay and not process redemptions during the period a redemption veto can still land. According to RFC-12, the tBTC wallets always consult the `WalletProposalValidator` to validate the ongoing redemption proposal before issuing the redemption transaction on the Bitcoin chain. That means it is enough to modify the `WalletProposalValidator.validateRedemptionProposal` function and take the veto delay into account there. The `validateRedemptionProposal` function calls the `RedemptionWatchtower.getRedemptionDelay` function for each redemption in the proposal and considers it valid only if the returned veto delay (counted since the redemption creation timestamp) already elapsed. This logic guarantees that a particular redemption proposal can only be considered valid if all of its redemption requests have surpassed their respective veto delay periods. --- solidity/contracts/bridge/Redemption.sol | 10 + .../contracts/bridge/RedemptionWatchtower.sol | 68 +++- .../bridge/WalletProposalValidator.sol | 21 +- .../test/bridge/RedemptionWatchtower.test.ts | 242 +++++++++++- .../bridge/WalletProposalValidator.test.ts | 368 +++++++++++++----- 5 files changed, 588 insertions(+), 121 deletions(-) diff --git a/solidity/contracts/bridge/Redemption.sol b/solidity/contracts/bridge/Redemption.sol index 4969848da..74ae1106b 100644 --- a/solidity/contracts/bridge/Redemption.sol +++ b/solidity/contracts/bridge/Redemption.sol @@ -43,6 +43,16 @@ interface IRedemptionWatchtower { address balanceOwner, address redeemer ) external view returns (bool); + + /// @notice Returns the applicable redemption delay for a redemption + /// request identified by the given redemption key. + /// @param redemptionKey Redemption key built as + /// `keccak256(keccak256(redeemerOutputScript) | walletPubKeyHash)`. + /// @return Redemption delay. + function getRedemptionDelay(uint256 redemptionKey) + external + view + returns (uint32); } /// @notice Aggregates functions common to the redemption transaction proof diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index f464fcfae..ea439f622 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -118,6 +118,11 @@ contract RedemptionWatchtower is OwnableUpgradeable { /// from the moment when the redemption request was created. /// Value in seconds. uint32 public levelTwoDelay; + /// @notice Limit of the redemption amount that can be waived from the + /// redemption veto mechanism. Redemption requests with the requested + /// amount lesser than this limit can be processed immediately and + /// cannot be subject of guardian objections. Value in satoshis. + uint64 public waivedAmountLimit; event WatchtowerEnabled(uint32 enabledAt, address manager); @@ -140,7 +145,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { uint32 vetoFreezePeriod, uint32 defaultDelay, uint32 levelOneDelay, - uint32 levelTwoDelay + uint32 levelTwoDelay, + uint64 waivedAmountLimit ); modifier onlyManager() { @@ -170,6 +176,7 @@ contract RedemptionWatchtower is OwnableUpgradeable { defaultDelay = 2 hours; levelOneDelay = 8 hours; levelTwoDelay = 24 hours; + waivedAmountLimit = 0; } /// @notice Enables the redemption watchtower and veto mechanism. @@ -299,7 +306,10 @@ contract RedemptionWatchtower is OwnableUpgradeable { /* solhint-disable-next-line not-rely-on-time */ block.timestamp < redemption.requestedAt + - _redemptionDelay(veto.objectionsCount), + _redemptionDelay( + veto.objectionsCount, + redemption.requestedAmount + ), "Redemption veto delay period expired" ); } else { @@ -344,25 +354,56 @@ contract RedemptionWatchtower is OwnableUpgradeable { } } - /// @notice Returns the redemption delay for a given number of objections. + /// @notice Returns the redemption delay for a given number of objections + /// and requested amount. /// @param objectionsCount Number of objections. - /// @return delay Redemption delay. - function _redemptionDelay(uint8 objectionsCount) + /// @param requestedAmount Requested redemption amount. + /// @return Redemption delay. + function _redemptionDelay(uint8 objectionsCount, uint64 requestedAmount) internal view - returns (uint32 delay) + returns (uint32) { + if (requestedAmount < waivedAmountLimit) { + return 0; + } + if (objectionsCount == 0) { - delay = defaultDelay; + return defaultDelay; } else if (objectionsCount == 1) { - delay = levelOneDelay; + return levelOneDelay; } else if (objectionsCount == 2) { - delay = levelTwoDelay; + return levelTwoDelay; } else { revert("No delay for given objections count"); } } + /// @notice Returns the applicable redemption delay for a redemption + /// request identified by the given redemption key. + /// @param redemptionKey Redemption key built as + /// `keccak256(keccak256(redeemerOutputScript) | walletPubKeyHash)`. + /// @return Redemption delay. + function getRedemptionDelay(uint256 redemptionKey) + external + view + returns (uint32) + { + Redemption.RedemptionRequest memory redemption = bridge + .pendingRedemptions(redemptionKey); + + require( + redemption.requestedAt != 0, + "Redemption request does not exist" + ); + + return + _redemptionDelay( + vetoProposals[redemptionKey].objectionsCount, + redemption.requestedAmount + ); + } + /// @notice Updates the watchtower parameters. /// @param _watchtowerLifetime Duration of the watchtower lifetime in seconds. /// @param _vetoPenaltyFeeDivisor Divisor used to compute the redemption veto @@ -373,6 +414,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { /// raised an objection to. /// @param _levelTwoDelay Delay applied to redemption requests two guardians /// raised an objection to. + /// @param _waivedAmountLimit Limit of the redemption amount that can be + /// waived from the redemption veto mechanism. /// @dev Requirements: /// - The caller must be the watchtower manager, /// - The new watchtower lifetime must not be lesser than the current one, @@ -385,7 +428,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { uint32 _vetoFreezePeriod, uint32 _defaultDelay, uint32 _levelOneDelay, - uint32 _levelTwoDelay + uint32 _levelTwoDelay, + uint64 _waivedAmountLimit ) external onlyManager { require( _watchtowerLifetime >= watchtowerLifetime, @@ -415,6 +459,7 @@ contract RedemptionWatchtower is OwnableUpgradeable { defaultDelay = _defaultDelay; levelOneDelay = _levelOneDelay; levelTwoDelay = _levelTwoDelay; + waivedAmountLimit = _waivedAmountLimit; emit WatchtowerParametersUpdated( _watchtowerLifetime, @@ -422,7 +467,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { _vetoFreezePeriod, _defaultDelay, _levelOneDelay, - _levelTwoDelay + _levelTwoDelay, + _waivedAmountLimit ); } diff --git a/solidity/contracts/bridge/WalletProposalValidator.sol b/solidity/contracts/bridge/WalletProposalValidator.sol index ae598e703..eee2eb74f 100644 --- a/solidity/contracts/bridge/WalletProposalValidator.sol +++ b/solidity/contracts/bridge/WalletProposalValidator.sol @@ -528,7 +528,9 @@ contract WalletProposalValidator { /// lesser than or equal to the maximum fee share allowed by the /// given request (`RedemptionRequest.txMaxFee`), /// - Each request must be a pending request registered in the Bridge, - /// - Each request must be old enough, i.e. at least `redemptionRequestMinAge` + /// - Each request must be old enough, i.e. at least `REDEMPTION_REQUEST_MIN_AGE` + /// OR the delay enforced by the redemption watchtower + /// (if the watchtower is set and the returned delay is greater than `REDEMPTION_REQUEST_MIN_AGE`) /// elapsed since their creation time, /// - Each request must have the timeout safety margin preserved, /// - Each request must be unique. @@ -582,6 +584,8 @@ contract WalletProposalValidator { uint256 redemptionTxFeePerRequest = (proposal.redemptionTxFee - redemptionTxFeeRemainder) / requestsCount; + address redemptionWatchtower = bridge.getRedemptionWatchtower(); + uint256[] memory processedRedemptionKeys = new uint256[](requestsCount); for (uint256 i = 0; i < requestsCount; i++) { @@ -608,12 +612,21 @@ contract WalletProposalValidator { "Not a pending redemption request" ); - // TODO: Validate the request against the RedemptionWatchtower. + uint32 minAge = REDEMPTION_REQUEST_MIN_AGE; + if (redemptionWatchtower != address(0)) { + // Check the redemption delay enforced by the watchtower. + uint32 delay = IRedemptionWatchtower(redemptionWatchtower) + .getRedemptionDelay(redemptionKey); + // If the delay is greater than the usual minimum age, use it. + // This way both the min age and the watchtower delay are preserved. + if (delay > minAge) { + minAge = delay; + } + } require( /* solhint-disable-next-line not-rely-on-time */ - block.timestamp > - redemptionRequest.requestedAt + REDEMPTION_REQUEST_MIN_AGE, + block.timestamp > redemptionRequest.requestedAt + minAge, "Redemption request min age not achieved yet" ); diff --git a/solidity/test/bridge/RedemptionWatchtower.test.ts b/solidity/test/bridge/RedemptionWatchtower.test.ts index 01380ff49..75d0ecebb 100644 --- a/solidity/test/bridge/RedemptionWatchtower.test.ts +++ b/solidity/test/bridge/RedemptionWatchtower.test.ts @@ -341,7 +341,8 @@ describe("RedemptionWatchtower", () => { await redemptionWatchtower.vetoFreezePeriod(), await redemptionWatchtower.defaultDelay(), await redemptionWatchtower.levelOneDelay(), - await redemptionWatchtower.levelTwoDelay() + await redemptionWatchtower.levelTwoDelay(), + await redemptionWatchtower.waivedAmountLimit() ) }) @@ -1298,6 +1299,212 @@ describe("RedemptionWatchtower", () => { }) }) + describe("getRedemptionDelay", () => { + before(async () => { + await createSnapshot() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the redemption request does not exist", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower.getRedemptionDelay( + "0xdf1d1a1cc88980461867cc49753d188c79c96c5a22115bdd8560a3317b786ff8" + ) + ).to.be.revertedWith("Redemption request does not exist") + }) + }) + + context("when the redemption request exists", () => { + let redemption: RedemptionData + + before(async () => { + await createSnapshot() + + const redemptions = await createRedemptionRequests( + SinglePendingRequestedRedemption + ) + // eslint-disable-next-line prefer-destructuring + redemption = redemptions[0] + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the requested amount is below the waived limit", () => { + before(async () => { + await createSnapshot() + + // Set the waived amount limit just above the redemption amount. + // This way the requested amount is 1 sat lesser than it. + await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + await redemptionWatchtower.watchtowerLifetime(), + 20, + await redemptionWatchtower.vetoFreezePeriod(), + await redemptionWatchtower.defaultDelay(), + await redemptionWatchtower.levelOneDelay(), + await redemptionWatchtower.levelTwoDelay(), + redemption.amount.add(1) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should return zero as the delay", async () => { + expect( + await redemptionWatchtower.getRedemptionDelay( + redemption.redemptionKey + ) + ).to.be.equal(0) + }) + }) + + context("when the requested amount is not below the waived limit", () => { + before(async () => { + await createSnapshot() + + // Set the waived amount limit to a value equal to the redemption amount. + // This way the requested amount violates the limit. + await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + await redemptionWatchtower.watchtowerLifetime(), + 20, + await redemptionWatchtower.vetoFreezePeriod(), + await redemptionWatchtower.defaultDelay(), + await redemptionWatchtower.levelOneDelay(), + await redemptionWatchtower.levelTwoDelay(), + redemption.amount + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when there are no objections", () => { + it("should return the default delay", async () => { + expect( + await redemptionWatchtower.getRedemptionDelay( + redemption.redemptionKey + ) + ).to.be.equal(await redemptionWatchtower.defaultDelay()) + }) + }) + + context("when there is one objection", () => { + before(async () => { + await createSnapshot() + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should return the level-one delay", async () => { + expect( + await redemptionWatchtower.getRedemptionDelay( + redemption.redemptionKey + ) + ).to.be.equal(await redemptionWatchtower.levelOneDelay()) + }) + }) + + context("when there are two objections", () => { + before(async () => { + await createSnapshot() + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + // Raise the second objection. + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should return the level-two delay", async () => { + expect( + await redemptionWatchtower.getRedemptionDelay( + redemption.redemptionKey + ) + ).to.be.equal(await redemptionWatchtower.levelTwoDelay()) + }) + }) + + context("when there are three objections", () => { + before(async () => { + await createSnapshot() + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + // Raise the second objection. + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + // Raise the third objection. + await redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower.getRedemptionDelay(redemption.redemptionKey) + ).to.be.revertedWith("Redemption request does not exist") + }) + }) + }) + }) + }) + describe("updateWatchtowerParameters", () => { let watchtowerLifetime: number let vetoPenaltyFeeDivisor: number @@ -1305,6 +1512,7 @@ describe("RedemptionWatchtower", () => { let defaultDelay: number let levelOneDelay: number let levelTwoDelay: number + let waivedAmountLimit: BigNumber before(async () => { await createSnapshot() @@ -1320,6 +1528,7 @@ describe("RedemptionWatchtower", () => { defaultDelay = await redemptionWatchtower.defaultDelay() levelOneDelay = await redemptionWatchtower.levelOneDelay() levelTwoDelay = await redemptionWatchtower.levelTwoDelay() + waivedAmountLimit = await redemptionWatchtower.waivedAmountLimit() }) after(async () => { @@ -1337,7 +1546,8 @@ describe("RedemptionWatchtower", () => { vetoFreezePeriod, defaultDelay, levelOneDelay, - levelTwoDelay + levelTwoDelay, + waivedAmountLimit ) ).to.be.revertedWith("Caller is not watchtower manager") }) @@ -1356,7 +1566,8 @@ describe("RedemptionWatchtower", () => { vetoFreezePeriod, defaultDelay, levelOneDelay, - levelTwoDelay + levelTwoDelay, + waivedAmountLimit ) ).to.be.revertedWith( "New lifetime must not be lesser than current one" @@ -1378,7 +1589,8 @@ describe("RedemptionWatchtower", () => { vetoFreezePeriod, defaultDelay, levelOneDelay, - levelTwoDelay + levelTwoDelay, + waivedAmountLimit ) ).to.be.revertedWith( "Redemption veto penalty fee must be in range [0%, 5%]" @@ -1398,7 +1610,8 @@ describe("RedemptionWatchtower", () => { vetoFreezePeriod, defaultDelay, levelOneDelay, - levelOneDelay - 1 + levelOneDelay - 1, + waivedAmountLimit ) ).to.be.revertedWith( "Redemption level-two delay must not be lesser than level-one delay" @@ -1417,7 +1630,8 @@ describe("RedemptionWatchtower", () => { vetoFreezePeriod, defaultDelay, defaultDelay - 1, - levelTwoDelay + levelTwoDelay, + waivedAmountLimit ) ).to.be.revertedWith( "Redemption level-one delay must not be lesser than default delay" @@ -1435,6 +1649,7 @@ describe("RedemptionWatchtower", () => { newDefaultDelay?: number newLevelOneDelay?: number newLevelTwoDelay?: number + newWaivedAmountLimit?: BigNumber }[] = [ { testName: "when watchtower lifetime is increased", @@ -1474,6 +1689,10 @@ describe("RedemptionWatchtower", () => { newLevelOneDelay: 0, newLevelTwoDelay: 0, }, + { + testName: "when waived amount limit is changed to a non-zero value", + newWaivedAmountLimit: BigNumber.from(50_000_000), + }, ] testData.forEach((test) => { @@ -1483,6 +1702,7 @@ describe("RedemptionWatchtower", () => { let newDefaultDelay: number let newLevelOneDelay: number let newLevelTwoDelay: number + let newWaivedAmountLimit: BigNumber context(test.testName, async () => { let tx: ContractTransaction @@ -1516,6 +1736,10 @@ describe("RedemptionWatchtower", () => { test.newLevelTwoDelay, levelTwoDelay ) + newWaivedAmountLimit = assignValue( + test.newWaivedAmountLimit, + waivedAmountLimit + ) tx = await redemptionWatchtower .connect(redemptionWatchtowerManager) @@ -1525,7 +1749,8 @@ describe("RedemptionWatchtower", () => { newVetoFreezePeriod, newDefaultDelay, newLevelOneDelay, - newLevelTwoDelay + newLevelTwoDelay, + newWaivedAmountLimit ) }) @@ -1542,7 +1767,8 @@ describe("RedemptionWatchtower", () => { newVetoFreezePeriod, newDefaultDelay, newLevelOneDelay, - newLevelTwoDelay + newLevelTwoDelay, + newWaivedAmountLimit ) }) diff --git a/solidity/test/bridge/WalletProposalValidator.test.ts b/solidity/test/bridge/WalletProposalValidator.test.ts index fa3e7ab5d..6fc873c79 100644 --- a/solidity/test/bridge/WalletProposalValidator.test.ts +++ b/solidity/test/bridge/WalletProposalValidator.test.ts @@ -3,13 +3,18 @@ import { ethers, helpers } from "hardhat" import chai, { expect } from "chai" import { FakeContract, smock } from "@defi-wonderland/smock" import { BigNumber, BigNumberish, BytesLike } from "ethers" -import type { Bridge, WalletProposalValidator } from "../../typechain" +import { arch } from "node:os" +import type { + Bridge, + IRedemptionWatchtower, + WalletProposalValidator, +} from "../../typechain" import { walletState, movedFundsSweepRequestState } from "../fixtures" import { NO_MAIN_UTXO } from "../data/deposit-sweep" chai.use(smock.matchers) -const { lastBlockTime } = helpers.time +const { lastBlockTime, increaseTime } = helpers.time const { createSnapshot, restoreSnapshot } = helpers.snapshot const { AddressZero, HashZero } = ethers.constants @@ -1477,65 +1482,173 @@ describe("WalletProposalValidator", () => { context("when all requests are pending", () => { context("when there is an immature request", () => { - let requestOne - let requestTwo + context( + "when immaturity is caused by REDEMPTION_REQUEST_MIN_AGE violation", + () => { + let requestOne + let requestTwo - before(async () => { - await createSnapshot() + before(async () => { + await createSnapshot() - requestOne = createTestRedemptionRequest( - walletPubKeyHash, - 5000 // necessary to pass the fee share validation - ) - requestTwo = createTestRedemptionRequest(walletPubKeyHash) + requestOne = createTestRedemptionRequest( + walletPubKeyHash, + 5000 // necessary to pass the fee share validation + ) + requestTwo = createTestRedemptionRequest(walletPubKeyHash) - // Request one is a proper one. - bridge.pendingRedemptions - .whenCalledWith( - redemptionKey( - requestOne.key.walletPubKeyHash, - requestOne.key.redeemerOutputScript + // Request one is a proper one. + bridge.pendingRedemptions + .whenCalledWith( + redemptionKey( + requestOne.key.walletPubKeyHash, + requestOne.key.redeemerOutputScript + ) + ) + .returns(requestOne.content) + + // Simulate the request two has just been created thus not + // achieved the min age yet. + bridge.pendingRedemptions + .whenCalledWith( + redemptionKey( + requestTwo.key.walletPubKeyHash, + requestTwo.key.redeemerOutputScript + ) + ) + .returns({ + ...requestTwo.content, + requestedAt: await lastBlockTime(), + }) + }) + + after(async () => { + bridge.pendingRedemptions.reset() + + await restoreSnapshot() + }) + + it("should revert", async () => { + const proposal = { + walletPubKeyHash, + redeemersOutputScripts: [ + requestOne.key.redeemerOutputScript, + requestTwo.key.redeemerOutputScript, + ], + redemptionTxFee, + } + + await expect( + walletProposalValidator.validateRedemptionProposal( + proposal + ) + ).to.be.revertedWith( + "Redemption request min age not achieved yet" ) - ) - .returns(requestOne.content) + }) + } + ) - // Simulate the request two has just been created thus not - // achieved the min age yet. - bridge.pendingRedemptions - .whenCalledWith( - redemptionKey( - requestTwo.key.walletPubKeyHash, - requestTwo.key.redeemerOutputScript + context( + "when immaturity is caused by watchtower's delay violation", + () => { + let watchtower: FakeContract + let requestOne + let requestTwo + + before(async () => { + await createSnapshot() + + requestOne = createTestRedemptionRequest( + walletPubKeyHash, + 5000, // necessary to pass the fee share validation + await lastBlockTime() ) - ) - .returns({ - ...requestTwo.content, - requestedAt: await lastBlockTime(), + requestTwo = createTestRedemptionRequest( + walletPubKeyHash, + 5000, // necessary to pass the fee share validation + await lastBlockTime() + ) + + // Request one is a proper one. + bridge.pendingRedemptions + .whenCalledWith( + redemptionKey( + requestOne.key.walletPubKeyHash, + requestOne.key.redeemerOutputScript + ) + ) + .returns(requestOne.content) + + // Simulate the request two has just been created thus not + // achieved the min age yet. + bridge.pendingRedemptions + .whenCalledWith( + redemptionKey( + requestTwo.key.walletPubKeyHash, + requestTwo.key.redeemerOutputScript + ) + ) + .returns(requestTwo.content) + + watchtower = await smock.fake( + "IRedemptionWatchtower" + ) + bridge.getRedemptionWatchtower.returns(watchtower.address) + + const redemptionOneDelay = 3600 + watchtower.getRedemptionDelay + .whenCalledWith( + buildRedemptionKey( + requestOne.key.walletPubKeyHash, + requestOne.key.redeemerOutputScript + ) + ) + .returns(redemptionOneDelay) + + const redemptionTwoDelay = 7200 + watchtower.getRedemptionDelay + .whenCalledWith( + buildRedemptionKey( + requestTwo.key.walletPubKeyHash, + requestTwo.key.redeemerOutputScript + ) + ) + .returns(redemptionTwoDelay) + + // Increase time to a point when delay for redemption + // one was elapsed but not for redemption two. + await increaseTime(redemptionTwoDelay) }) - }) - after(async () => { - bridge.pendingRedemptions.reset() + after(async () => { + bridge.getRedemptionWatchtower.reset() + watchtower.getRedemptionDelay.reset() + bridge.pendingRedemptions.reset() - await restoreSnapshot() - }) + await restoreSnapshot() + }) - it("should revert", async () => { - const proposal = { - walletPubKeyHash, - redeemersOutputScripts: [ - requestOne.key.redeemerOutputScript, - requestTwo.key.redeemerOutputScript, - ], - redemptionTxFee, - } + it("should revert", async () => { + const proposal = { + walletPubKeyHash, + redeemersOutputScripts: [ + requestOne.key.redeemerOutputScript, + requestTwo.key.redeemerOutputScript, + ], + redemptionTxFee, + } - await expect( - walletProposalValidator.validateRedemptionProposal(proposal) - ).to.be.revertedWith( - "Redemption request min age not achieved yet" - ) - }) + await expect( + walletProposalValidator.validateRedemptionProposal( + proposal + ) + ).to.be.revertedWith( + "Redemption request min age not achieved yet" + ) + }) + } + ) }) context("when all requests achieved the min age", () => { @@ -1846,64 +1959,111 @@ describe("WalletProposalValidator", () => { }) context("when all requests are unique", () => { - let requestOne - let requestTwo - - before(async () => { - await createSnapshot() + const testData: { + testName: string + watchtower: boolean + }[] = [ + { + testName: "when watchtower is not set", + watchtower: false, + }, + { + testName: "when watchtower is set", + watchtower: true, + }, + ] + + testData.forEach((test) => { + context(test.testName, () => { + let watchtower: FakeContract + let requestOne + let requestTwo - requestOne = createTestRedemptionRequest( - walletPubKeyHash, - 5000 // necessary to pass the fee share validation - ) - - requestTwo = createTestRedemptionRequest( - walletPubKeyHash, - 5000 // necessary to pass the fee share validation - ) + before(async () => { + await createSnapshot() - bridge.pendingRedemptions - .whenCalledWith( - redemptionKey( - requestOne.key.walletPubKeyHash, - requestOne.key.redeemerOutputScript + requestOne = createTestRedemptionRequest( + walletPubKeyHash, + 5000 // necessary to pass the fee share validation ) - ) - .returns(requestOne.content) - bridge.pendingRedemptions - .whenCalledWith( - redemptionKey( - requestTwo.key.walletPubKeyHash, - requestTwo.key.redeemerOutputScript + requestTwo = createTestRedemptionRequest( + walletPubKeyHash, + 5000 // necessary to pass the fee share validation ) - ) - .returns(requestTwo.content) - }) - after(async () => { - bridge.pendingRedemptions.reset() + bridge.pendingRedemptions + .whenCalledWith( + redemptionKey( + requestOne.key.walletPubKeyHash, + requestOne.key.redeemerOutputScript + ) + ) + .returns(requestOne.content) - await restoreSnapshot() - }) + bridge.pendingRedemptions + .whenCalledWith( + redemptionKey( + requestTwo.key.walletPubKeyHash, + requestTwo.key.redeemerOutputScript + ) + ) + .returns(requestTwo.content) - it("should succeed", async () => { - const proposal = { - walletPubKeyHash, - redeemersOutputScripts: [ - requestOne.key.redeemerOutputScript, - requestTwo.key.redeemerOutputScript, - ], - redemptionTxFee, - } + if (test.watchtower) { + watchtower = + await smock.fake( + "IRedemptionWatchtower" + ) - const result = - await walletProposalValidator.validateRedemptionProposal( - proposal - ) + bridge.getRedemptionWatchtower.returns( + watchtower.address + ) - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - expect(result).to.be.true + // All requests created by createTestRedemptionRequest + // are requested at `now - 1 day` by default. + // To test the watchtower delay path, we need + // to use a delay that is greater than + // `REDEMPTION_REQUEST_MIN_AGE` (10 min) and + // ensure that the delay is preserved + // at the moment of the proposal validation. + // A value of 2 hours will be a good fit. + watchtower.getRedemptionDelay.returns( + 7200 // 2 hours + ) + } + }) + + after(async () => { + if (test.watchtower) { + bridge.getRedemptionWatchtower.reset() + watchtower.getRedemptionDelay.reset() + } + + bridge.pendingRedemptions.reset() + + await restoreSnapshot() + }) + + it("should succeed", async () => { + const proposal = { + walletPubKeyHash, + redeemersOutputScripts: [ + requestOne.key.redeemerOutputScript, + requestTwo.key.redeemerOutputScript, + ], + redemptionTxFee, + } + + const result = + await walletProposalValidator.validateRedemptionProposal( + proposal + ) + + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect(result).to.be.true + }) + }) }) }) } @@ -2796,3 +2956,15 @@ const movedFundsSweepRequestKey = ( ["bytes32", "uint32"], [movingFundsTxHash, movingFundsTxOutputIndex] ) + +const buildRedemptionKey = ( + walletPubKeyHash: BytesLike, + redeemerOutputScript: BytesLike +): string => + ethers.utils.solidityKeccak256( + ["bytes32", "bytes20"], + [ + ethers.utils.solidityKeccak256(["bytes"], [redeemerOutputScript]), + walletPubKeyHash, + ] + ) From 8d7d51b7eb6b3a3f461684de057f43e50cb0934c Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 21 Feb 2024 13:55:17 +0100 Subject: [PATCH 14/23] Expose the `disableWatchtower` function As part of the social contract, we are exposing the `disableWatchtower` function that can be used by anyone to disable the veto mechanism once the watchtower's lifetime expires (18 months by default). --- .../contracts/bridge/RedemptionWatchtower.sol | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index ea439f622..0199becba 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -126,6 +126,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { event WatchtowerEnabled(uint32 enabledAt, address manager); + event WatchtowerDisabled(uint32 disabledAt, address executor); + event GuardianAdded(address indexed guardian); event GuardianRemoved(address indexed guardian); @@ -206,6 +208,30 @@ contract RedemptionWatchtower is OwnableUpgradeable { emit WatchtowerEnabled(enabledAt, _manager); } + /// @notice Disables the redemption watchtower and veto mechanism. + /// Can be called by anyone. + /// @dev Requirements: + /// - Watchtower must be enabled, + /// - Watchtower must not be disabled already, + /// - Watchtower lifetime must have expired. + function disableWatchtower() external { + require(watchtowerEnabledAt != 0, "Not enabled"); + + require(watchtowerDisabledAt == 0, "Already disabled"); + + require( + /* solhint-disable-next-line not-rely-on-time */ + block.timestamp > watchtowerEnabledAt + watchtowerLifetime, + "Watchtower lifetime not expired" + ); + + /* solhint-disable-next-line not-rely-on-time */ + uint32 disabledAt = uint32(block.timestamp); + watchtowerDisabledAt = disabledAt; + + emit WatchtowerDisabled(disabledAt, msg.sender); + } + /// @notice Adds a redemption guardian /// @param guardian Address of the guardian to add. /// @dev Requirements: @@ -303,6 +329,9 @@ contract RedemptionWatchtower is OwnableUpgradeable { // any time restrictions. if (redemption.requestedAt >= watchtowerEnabledAt) { require( + // Use < instead of <= to avoid a theoretical edge case + // where the delay is 0 (veto disabled) but three objections + // are raised in the same block as the redemption request. /* solhint-disable-next-line not-rely-on-time */ block.timestamp < redemption.requestedAt + @@ -359,11 +388,17 @@ contract RedemptionWatchtower is OwnableUpgradeable { /// @param objectionsCount Number of objections. /// @param requestedAmount Requested redemption amount. /// @return Redemption delay. + /// @dev If the watchtower has been disabled, the delay is always zero, + /// for any redemption request. function _redemptionDelay(uint8 objectionsCount, uint64 requestedAmount) internal view returns (uint32) { + if (watchtowerDisabledAt != 0) { + return 0; + } + if (requestedAmount < waivedAmountLimit) { return 0; } @@ -384,6 +419,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { /// @param redemptionKey Redemption key built as /// `keccak256(keccak256(redeemerOutputScript) | walletPubKeyHash)`. /// @return Redemption delay. + /// @dev If the watchtower has been disabled, the delay is always zero, + /// for any redemption request. function getRedemptionDelay(uint256 redemptionKey) external view From 743e9c752e0e7519e3d629f203b007bafc55f747 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 21 Feb 2024 13:59:21 +0100 Subject: [PATCH 15/23] Add explanation about `setRedemptionWatchtower` function --- solidity/contracts/bridge/Bridge.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/solidity/contracts/bridge/Bridge.sol b/solidity/contracts/bridge/Bridge.sol index d54aeb350..0db637962 100644 --- a/solidity/contracts/bridge/Bridge.sol +++ b/solidity/contracts/bridge/Bridge.sol @@ -1957,6 +1957,7 @@ contract Bridge is external onlyGovernance { + // The internal function is defined in the `BridgeState` library. self.setRedemptionWatchtower(redemptionWatchtower); } From 6b0a76abb74a5bde607bed4b68c7df3f699cdd49 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 21 Feb 2024 14:40:10 +0100 Subject: [PATCH 16/23] Unit test covering the case when manager is not set and `addGuardian` is called --- .../test/bridge/RedemptionWatchtower.test.ts | 94 +++++++++++-------- 1 file changed, 54 insertions(+), 40 deletions(-) diff --git a/solidity/test/bridge/RedemptionWatchtower.test.ts b/solidity/test/bridge/RedemptionWatchtower.test.ts index 75d0ecebb..1dde717c1 100644 --- a/solidity/test/bridge/RedemptionWatchtower.test.ts +++ b/solidity/test/bridge/RedemptionWatchtower.test.ts @@ -172,65 +172,79 @@ describe("RedemptionWatchtower", () => { }) describe("addGuardian", () => { - before(async () => { - await createSnapshot() - - await redemptionWatchtower.connect(governance).enableWatchtower( - redemptionWatchtowerManager.address, - guardians.map((g) => g.address) - ) - }) - - after(async () => { - await restoreSnapshot() - }) - - context("when called not by the watchtower manager", () => { + context("when watchtower manager is not set", () => { + // At this point, the watchtower manager is not set as `enableWatchtower` + // has not been called yet. it("should revert", async () => { await expect( redemptionWatchtower - .connect(governance) // governance has not such a power + .connect(thirdParty) .addGuardian(thirdParty.address) ).to.be.revertedWith("Caller is not watchtower manager") }) }) - context("when called by the watchtower manager", () => { - context("when guardian already exists", () => { + context("when watchtower manager is set", () => { + before(async () => { + await createSnapshot() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when called not by the watchtower manager", () => { it("should revert", async () => { await expect( redemptionWatchtower - .connect(redemptionWatchtowerManager) - .addGuardian(guardians[0].address) - ).to.be.revertedWith("Guardian already exists") + .connect(governance) // governance has not such a power + .addGuardian(thirdParty.address) + ).to.be.revertedWith("Caller is not watchtower manager") }) }) - context("when guardian does not exist", () => { - let tx: ContractTransaction + context("when called by the watchtower manager", () => { + context("when guardian already exists", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redemptionWatchtowerManager) + .addGuardian(guardians[0].address) + ).to.be.revertedWith("Guardian already exists") + }) + }) - before(async () => { - await createSnapshot() + context("when guardian does not exist", () => { + let tx: ContractTransaction - tx = await redemptionWatchtower - .connect(redemptionWatchtowerManager) - .addGuardian(thirdParty.address) - }) + before(async () => { + await createSnapshot() - after(async () => { - await restoreSnapshot() - }) + tx = await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .addGuardian(thirdParty.address) + }) - it("should add the guardian properly", async () => { - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - expect(await redemptionWatchtower.isGuardian(thirdParty.address)).to - .be.true - }) + after(async () => { + await restoreSnapshot() + }) - it("should emit GuardianAdded event", async () => { - await expect(tx) - .to.emit(redemptionWatchtower, "GuardianAdded") - .withArgs(thirdParty.address) + it("should add the guardian properly", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect(await redemptionWatchtower.isGuardian(thirdParty.address)).to + .be.true + }) + + it("should emit GuardianAdded event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "GuardianAdded") + .withArgs(thirdParty.address) + }) }) }) }) From 584c4bb976c521a8f4def415a4e2f04aee05936f Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 21 Feb 2024 14:48:29 +0100 Subject: [PATCH 17/23] Unit test for `disableWatchtower` function --- .../test/bridge/RedemptionWatchtower.test.ts | 486 +++++++++++++----- 1 file changed, 368 insertions(+), 118 deletions(-) diff --git a/solidity/test/bridge/RedemptionWatchtower.test.ts b/solidity/test/bridge/RedemptionWatchtower.test.ts index 1dde717c1..36c561726 100644 --- a/solidity/test/bridge/RedemptionWatchtower.test.ts +++ b/solidity/test/bridge/RedemptionWatchtower.test.ts @@ -129,6 +129,12 @@ describe("RedemptionWatchtower", () => { await restoreSnapshot() }) + it("should set the enabledAt timeout properly", async () => { + expect(await redemptionWatchtower.watchtowerEnabledAt()).to.equal( + await lastBlockTime() + ) + }) + it("should set the watchtower manager properly", async () => { expect(await redemptionWatchtower.manager()).to.equal( redemptionWatchtowerManager.address @@ -171,6 +177,124 @@ describe("RedemptionWatchtower", () => { }) }) + describe("disableWatchtower", () => { + context("when the watchtower is not enabled", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower.connect(thirdParty).disableWatchtower() + ).to.be.revertedWith("Not enabled") + }) + }) + + context("when the watchtower is enabled", () => { + before(async () => { + await createSnapshot() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the watchtower is disabled already", () => { + before(async () => { + await createSnapshot() + + const lifetimeExpiresAt = + (await redemptionWatchtower.watchtowerEnabledAt()) + + (await redemptionWatchtower.watchtowerLifetime()) + + // Increase time to the moment the watchtower lifetime expires. + // The `disableWatchtower` transaction should be mined exactly one + // second after the lifetime expires. + await increaseTime(lifetimeExpiresAt - (await lastBlockTime())) + + // Disable the watchtower for the first time. + await redemptionWatchtower.connect(thirdParty).disableWatchtower() + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower.connect(thirdParty).disableWatchtower() + ).to.be.revertedWith("Already disabled") + }) + }) + + context("when the watchtower is not disabled yet", () => { + context("when the watchtower lifetime is not expired", () => { + before(async () => { + await createSnapshot() + + const lifetimeExpiresAt = + (await redemptionWatchtower.watchtowerEnabledAt()) + + (await redemptionWatchtower.watchtowerLifetime()) + + // Increase time to one second before the watchtower lifetime expires. + // The `disableWatchtower` will be mined exactly at the moment + // of the lifetime expiration which is one second too early + // to disable the watchtower. + await increaseTime(lifetimeExpiresAt - (await lastBlockTime()) - 1) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower.connect(thirdParty).disableWatchtower() + ).to.be.revertedWith("Watchtower lifetime not expired") + }) + }) + + context("when the watchtower lifetime is expired", () => { + let tx: ContractTransaction + + before(async () => { + await createSnapshot() + + const lifetimeExpiresAt = + (await redemptionWatchtower.watchtowerEnabledAt()) + + (await redemptionWatchtower.watchtowerLifetime()) + + // Increase time to the moment the watchtower lifetime expires. + // The `disableWatchtower` transaction should be mined exactly one + // second after the lifetime expires. + await increaseTime(lifetimeExpiresAt - (await lastBlockTime())) + + tx = await redemptionWatchtower + .connect(thirdParty) + .disableWatchtower() + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should set the disabledAt timeout properly", async () => { + expect(await redemptionWatchtower.watchtowerDisabledAt()).to.equal( + await lastBlockTime() + ) + }) + + it("should emit WatchtowerDisabled event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "WatchtowerDisabled") + .withArgs(await lastBlockTime(), thirdParty.address) + }) + }) + }) + }) + }) + describe("addGuardian", () => { context("when watchtower manager is not set", () => { // At this point, the watchtower manager is not set as `enableWatchtower` @@ -494,6 +618,95 @@ describe("RedemptionWatchtower", () => { }) context("when redemption request exists", () => { + context( + "when the requested amount is below the waived amount limit", + () => { + let redemption: RedemptionData + + before(async () => { + await createSnapshot() + + const redemptions = await createRedemptionRequests( + SinglePendingRequestedRedemption + ) + // eslint-disable-next-line prefer-destructuring + redemption = redemptions[0] + + // Set the waived amount limit just above the redemption amount. + // This way the requested amount is 1 sat lesser than it. + await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + await redemptionWatchtower.watchtowerLifetime(), + 20, + await redemptionWatchtower.vetoFreezePeriod(), + await redemptionWatchtower.defaultDelay(), + await redemptionWatchtower.levelOneDelay(), + await redemptionWatchtower.levelTwoDelay(), + redemption.amount.add(1) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + ).to.be.revertedWith("Redemption veto delay period expired") + }) + } + ) + + context("when watchtower has been disabled", () => { + let redemption: RedemptionData + + before(async () => { + await createSnapshot() + + const redemptions = await createRedemptionRequests( + SinglePendingRequestedRedemption + ) + // eslint-disable-next-line prefer-destructuring + redemption = redemptions[0] + + const lifetimeExpiresAt = + (await redemptionWatchtower.watchtowerEnabledAt()) + + (await redemptionWatchtower.watchtowerLifetime()) + + // Increase time to the moment the watchtower lifetime expires. + // The `disableWatchtower` transaction should be mined exactly one + // second after the lifetime expires. + await increaseTime(lifetimeExpiresAt - (await lastBlockTime())) + + // Disable the watchtower. + await redemptionWatchtower + .connect(thirdParty) + .disableWatchtower() + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + ).to.be.revertedWith("Redemption veto delay period expired") + }) + }) + context( "when delay period expired and request was created after mechanism initialization", () => { @@ -1354,23 +1567,21 @@ describe("RedemptionWatchtower", () => { await restoreSnapshot() }) - context("when the requested amount is below the waived limit", () => { + context("when the watchtower has been disabled", () => { before(async () => { await createSnapshot() - // Set the waived amount limit just above the redemption amount. - // This way the requested amount is 1 sat lesser than it. - await redemptionWatchtower - .connect(redemptionWatchtowerManager) - .updateWatchtowerParameters( - await redemptionWatchtower.watchtowerLifetime(), - 20, - await redemptionWatchtower.vetoFreezePeriod(), - await redemptionWatchtower.defaultDelay(), - await redemptionWatchtower.levelOneDelay(), - await redemptionWatchtower.levelTwoDelay(), - redemption.amount.add(1) - ) + const lifetimeExpiresAt = + (await redemptionWatchtower.watchtowerEnabledAt()) + + (await redemptionWatchtower.watchtowerLifetime()) + + // Increase time to the moment the watchtower lifetime expires. + // The `disableWatchtower` transaction should be mined exactly one + // second after the lifetime expires. + await increaseTime(lifetimeExpiresAt - (await lastBlockTime())) + + // Disable the watchtower. + await redemptionWatchtower.connect(thirdParty).disableWatchtower() }) after(async () => { @@ -1386,49 +1597,23 @@ describe("RedemptionWatchtower", () => { }) }) - context("when the requested amount is not below the waived limit", () => { - before(async () => { - await createSnapshot() - - // Set the waived amount limit to a value equal to the redemption amount. - // This way the requested amount violates the limit. - await redemptionWatchtower - .connect(redemptionWatchtowerManager) - .updateWatchtowerParameters( - await redemptionWatchtower.watchtowerLifetime(), - 20, - await redemptionWatchtower.vetoFreezePeriod(), - await redemptionWatchtower.defaultDelay(), - await redemptionWatchtower.levelOneDelay(), - await redemptionWatchtower.levelTwoDelay(), - redemption.amount - ) - }) - - after(async () => { - await restoreSnapshot() - }) - - context("when there are no objections", () => { - it("should return the default delay", async () => { - expect( - await redemptionWatchtower.getRedemptionDelay( - redemption.redemptionKey - ) - ).to.be.equal(await redemptionWatchtower.defaultDelay()) - }) - }) - - context("when there is one objection", () => { + context("when the watchtower has not been disabled", () => { + context("when the requested amount is below the waived limit", () => { before(async () => { await createSnapshot() - // Raise the first objection. + // Set the waived amount limit just above the redemption amount. + // This way the requested amount is 1 sat lesser than it. await redemptionWatchtower - .connect(guardians[0]) - .raiseObjection( - redemption.walletPublicKeyHash, - redemption.redeemerOutputScript + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + await redemptionWatchtower.watchtowerLifetime(), + 20, + await redemptionWatchtower.vetoFreezePeriod(), + await redemptionWatchtower.defaultDelay(), + await redemptionWatchtower.levelOneDelay(), + await redemptionWatchtower.levelTwoDelay(), + redemption.amount.add(1) ) }) @@ -1436,85 +1621,150 @@ describe("RedemptionWatchtower", () => { await restoreSnapshot() }) - it("should return the level-one delay", async () => { + it("should return zero as the delay", async () => { expect( await redemptionWatchtower.getRedemptionDelay( redemption.redemptionKey ) - ).to.be.equal(await redemptionWatchtower.levelOneDelay()) + ).to.be.equal(0) }) }) - context("when there are two objections", () => { - before(async () => { - await createSnapshot() + context( + "when the requested amount is not below the waived limit", + () => { + before(async () => { + await createSnapshot() - // Raise the first objection. - await redemptionWatchtower - .connect(guardians[0]) - .raiseObjection( - redemption.walletPublicKeyHash, - redemption.redeemerOutputScript - ) - // Raise the second objection. - await redemptionWatchtower - .connect(guardians[1]) - .raiseObjection( - redemption.walletPublicKeyHash, - redemption.redeemerOutputScript - ) - }) + // Set the waived amount limit to a value equal to the redemption amount. + // This way the requested amount violates the limit. + await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + await redemptionWatchtower.watchtowerLifetime(), + 20, + await redemptionWatchtower.vetoFreezePeriod(), + await redemptionWatchtower.defaultDelay(), + await redemptionWatchtower.levelOneDelay(), + await redemptionWatchtower.levelTwoDelay(), + redemption.amount + ) + }) - after(async () => { - await restoreSnapshot() - }) + after(async () => { + await restoreSnapshot() + }) - it("should return the level-two delay", async () => { - expect( - await redemptionWatchtower.getRedemptionDelay( - redemption.redemptionKey - ) - ).to.be.equal(await redemptionWatchtower.levelTwoDelay()) - }) - }) + context("when there are no objections", () => { + it("should return the default delay", async () => { + expect( + await redemptionWatchtower.getRedemptionDelay( + redemption.redemptionKey + ) + ).to.be.equal(await redemptionWatchtower.defaultDelay()) + }) + }) - context("when there are three objections", () => { - before(async () => { - await createSnapshot() + context("when there is one objection", () => { + before(async () => { + await createSnapshot() - // Raise the first objection. - await redemptionWatchtower - .connect(guardians[0]) - .raiseObjection( - redemption.walletPublicKeyHash, - redemption.redeemerOutputScript - ) - // Raise the second objection. - await redemptionWatchtower - .connect(guardians[1]) - .raiseObjection( - redemption.walletPublicKeyHash, - redemption.redeemerOutputScript - ) - // Raise the third objection. - await redemptionWatchtower - .connect(guardians[2]) - .raiseObjection( - redemption.walletPublicKeyHash, - redemption.redeemerOutputScript - ) - }) + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) - after(async () => { - await restoreSnapshot() - }) + after(async () => { + await restoreSnapshot() + }) - it("should revert", async () => { - await expect( - redemptionWatchtower.getRedemptionDelay(redemption.redemptionKey) - ).to.be.revertedWith("Redemption request does not exist") - }) - }) + it("should return the level-one delay", async () => { + expect( + await redemptionWatchtower.getRedemptionDelay( + redemption.redemptionKey + ) + ).to.be.equal(await redemptionWatchtower.levelOneDelay()) + }) + }) + + context("when there are two objections", () => { + before(async () => { + await createSnapshot() + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + // Raise the second objection. + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should return the level-two delay", async () => { + expect( + await redemptionWatchtower.getRedemptionDelay( + redemption.redemptionKey + ) + ).to.be.equal(await redemptionWatchtower.levelTwoDelay()) + }) + }) + + context("when there are three objections", () => { + before(async () => { + await createSnapshot() + + // Raise the first objection. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + // Raise the second objection. + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + // Raise the third objection. + await redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower.getRedemptionDelay( + redemption.redemptionKey + ) + ).to.be.revertedWith("Redemption request does not exist") + }) + }) + } + ) }) }) }) From d3cc2e8e801569f0459453aebc7132ed711cdf02 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 21 Feb 2024 14:53:20 +0100 Subject: [PATCH 18/23] Suppress Slither's false positives --- .../contracts/bridge/RedemptionWatchtower.sol | 17 ++++++++++++++--- .../bridge/WalletProposalValidator.sol | 1 + .../test/bridge/WalletProposalValidator.test.ts | 1 - 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index 0199becba..486d5ae1c 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -32,6 +32,7 @@ import "./Redemption.sol"; /// requester while inflicting a freeze and financial penalty on the /// amount. The goal of this penalty is to introduce a cost that guards /// against repeated malicious redemption requests. +// slither-disable-next-line missing-inheritance contract RedemptionWatchtower is OwnableUpgradeable { struct VetoProposal { // Address of the redeemer that requested the redemption. @@ -217,6 +218,7 @@ contract RedemptionWatchtower is OwnableUpgradeable { function disableWatchtower() external { require(watchtowerEnabledAt != 0, "Not enabled"); + // slither-disable-next-line incorrect-equality require(watchtowerDisabledAt == 0, "Already disabled"); require( @@ -403,11 +405,20 @@ contract RedemptionWatchtower is OwnableUpgradeable { return 0; } - if (objectionsCount == 0) { + if ( + // slither-disable-next-line incorrect-equality + objectionsCount == 0 + ) { return defaultDelay; - } else if (objectionsCount == 1) { + } else if ( + // slither-disable-next-line incorrect-equality + objectionsCount == 1 + ) { return levelOneDelay; - } else if (objectionsCount == 2) { + } else if ( + // slither-disable-next-line incorrect-equality + objectionsCount == 2 + ) { return levelTwoDelay; } else { revert("No delay for given objections count"); diff --git a/solidity/contracts/bridge/WalletProposalValidator.sol b/solidity/contracts/bridge/WalletProposalValidator.sol index eee2eb74f..683027a67 100644 --- a/solidity/contracts/bridge/WalletProposalValidator.sol +++ b/solidity/contracts/bridge/WalletProposalValidator.sol @@ -615,6 +615,7 @@ contract WalletProposalValidator { uint32 minAge = REDEMPTION_REQUEST_MIN_AGE; if (redemptionWatchtower != address(0)) { // Check the redemption delay enforced by the watchtower. + // slither-disable-next-line calls-loop uint32 delay = IRedemptionWatchtower(redemptionWatchtower) .getRedemptionDelay(redemptionKey); // If the delay is greater than the usual minimum age, use it. diff --git a/solidity/test/bridge/WalletProposalValidator.test.ts b/solidity/test/bridge/WalletProposalValidator.test.ts index 6fc873c79..8b29944e8 100644 --- a/solidity/test/bridge/WalletProposalValidator.test.ts +++ b/solidity/test/bridge/WalletProposalValidator.test.ts @@ -3,7 +3,6 @@ import { ethers, helpers } from "hardhat" import chai, { expect } from "chai" import { FakeContract, smock } from "@defi-wonderland/smock" import { BigNumber, BigNumberish, BytesLike } from "ethers" -import { arch } from "node:os" import type { Bridge, IRedemptionWatchtower, From 7425eba75298a44de440c1ad4067cccd71b313e8 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 21 Feb 2024 15:07:42 +0100 Subject: [PATCH 19/23] Expose `unban` function This function is a safeguard in cases where honest redeemers are banned due to guardians' mistake/malice. --- .../contracts/bridge/RedemptionWatchtower.sol | 17 +++ .../test/bridge/RedemptionWatchtower.test.ts | 102 ++++++++++++++++++ 2 files changed, 119 insertions(+) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index 486d5ae1c..29703a3fa 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -152,6 +152,10 @@ contract RedemptionWatchtower is OwnableUpgradeable { uint64 waivedAmountLimit ); + event Banned(address indexed redeemer); + + event Unbanned(address indexed redeemer); + modifier onlyManager() { require(msg.sender == manager, "Caller is not watchtower manager"); _; @@ -372,6 +376,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { // requests from that address. isBanned[redemption.redeemer] = true; + emit Banned(redemption.redeemer); + emit VetoFinalized(redemptionKey); // Notify the Bridge about the veto. As result of this call, @@ -560,4 +566,15 @@ contract RedemptionWatchtower is OwnableUpgradeable { return true; } + + /// @notice Unbans a redeemer. + /// @param redeemer Address of the redeemer to unban. + /// @dev Requirements: + /// - The caller must be the watchtower manager, + /// - The redeemer must be banned. + function unban(address redeemer) external onlyManager { + require(isBanned[redeemer], "Redeemer is not banned"); + isBanned[redeemer] = false; + emit Unbanned(redeemer); + } } diff --git a/solidity/test/bridge/RedemptionWatchtower.test.ts b/solidity/test/bridge/RedemptionWatchtower.test.ts index 36c561726..8336d078c 100644 --- a/solidity/test/bridge/RedemptionWatchtower.test.ts +++ b/solidity/test/bridge/RedemptionWatchtower.test.ts @@ -1103,6 +1103,12 @@ describe("RedemptionWatchtower", () => { ).to.be.true }) + it("should emit Banned event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "Banned") + .withArgs(legacyRedemption.redeemer) + }) + it("should emit VetoFinalized event", async () => { await expect(tx) .to.emit(redemptionWatchtower, "VetoFinalized") @@ -1446,6 +1452,12 @@ describe("RedemptionWatchtower", () => { ).to.be.true }) + it("should emit Banned event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "Banned") + .withArgs(redemption.redeemer) + }) + it("should emit VetoFinalized event", async () => { await expect(tx) .to.emit(redemptionWatchtower, "VetoFinalized") @@ -2207,6 +2219,96 @@ describe("RedemptionWatchtower", () => { }) }) + describe("unban", () => { + before(async () => { + await createSnapshot() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the caller is not the watchtower manager", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower.connect(thirdParty).unban(thirdParty.address) + ).to.be.revertedWith("Caller is not watchtower manager") + }) + }) + + context("when the caller is the watchtower manager", () => { + context("when the redeemer is not banned", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redemptionWatchtowerManager) + .unban(thirdParty.address) + ).to.be.revertedWith("Redeemer is not banned") + }) + }) + + context("when the redeemer is banned", () => { + let tx: ContractTransaction + let redemption: RedemptionData + + before(async () => { + await createSnapshot() + + const redemptions = await createRedemptionRequests( + SinglePendingRequestedRedemption + ) + // eslint-disable-next-line prefer-destructuring + redemption = redemptions[0] + + // Raise three objections to veto the redemption and ban the redeemer. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + await redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + tx = await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .unban(redemption.redeemer) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should remove the redeemer from the banned list", async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + expect(await redemptionWatchtower.isBanned(redemption.redeemer)).to.be + .false + }) + + it("should emit Unbanned event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "Unbanned") + .withArgs(redemption.redeemer) + }) + }) + }) + }) + type RedemptionData = { redemptionKey: string walletPublicKeyHash: string From 6a2932d92014822664a78512bd729358262f9be4 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 21 Feb 2024 16:19:16 +0100 Subject: [PATCH 20/23] Expose `withdrawVetoedFunds` function Here we expose the `withdrawVetoedFunds` allowing the redeemer to withdraw their funds from vetoed redemption, once the freeze period expires. Withdrawn amount is equal to the redemption requested amount diminished by the current value of the penalty fee. --- .../contracts/bridge/RedemptionWatchtower.sol | 44 +- .../test/bridge/RedemptionWatchtower.test.ts | 430 +++++++++++++++++- 2 files changed, 461 insertions(+), 13 deletions(-) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index 29703a3fa..80f043e06 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -37,10 +37,10 @@ contract RedemptionWatchtower is OwnableUpgradeable { struct VetoProposal { // Address of the redeemer that requested the redemption. address redeemer; - // Amount that the redeemer can claim after the freeze period. + // Amount that the redeemer can withdraw after the freeze period. // Value is 0 if the veto is not finalized or the amount was // already claimed. - uint64 claimableAmount; + uint64 withdrawableAmount; // Timestamp when the veto was finalized. Value is 0 if the veto is // not finalized. uint32 finalizedAt; @@ -156,6 +156,12 @@ contract RedemptionWatchtower is OwnableUpgradeable { event Unbanned(address indexed redeemer); + event VetoedFundsWithdrawn( + uint256 indexed redemptionkey, + address indexed redeemer, + uint64 amount + ); + modifier onlyManager() { require(msg.sender == manager, "Caller is not watchtower manager"); _; @@ -369,7 +375,7 @@ contract RedemptionWatchtower is OwnableUpgradeable { : 0; // Set finalization fields in the veto request. - veto.claimableAmount = redemption.requestedAmount - penaltyFee; + veto.withdrawableAmount = redemption.requestedAmount - penaltyFee; /* solhint-disable-next-line not-rely-on-time */ veto.finalizedAt = uint32(block.timestamp); // Mark the redeemer as banned to prevent future redemption @@ -577,4 +583,36 @@ contract RedemptionWatchtower is OwnableUpgradeable { isBanned[redeemer] = false; emit Unbanned(redeemer); } + + /// @notice Withdraws funds from a vetoed redemption request, identified + /// by the given redemption key. + /// @param redemptionKey Redemption key built as + /// `keccak256(keccak256(redeemerOutputScript) | walletPubKeyHash)`. + /// @dev Requirements: + /// - The veto must be finalized, + /// - The caller must be the redeemer of the vetoed redemption request, + /// - The freeze period must have expired, + /// - There must be funds to withdraw. + function withdrawVetoedFunds(uint256 redemptionKey) external { + VetoProposal storage veto = vetoProposals[redemptionKey]; + + require(veto.finalizedAt != 0, "Redemption veto not finalized"); + + require(msg.sender == veto.redeemer, "Caller is not the redeemer"); + + require( + /* solhint-disable-next-line not-rely-on-time */ + block.timestamp > veto.finalizedAt + vetoFreezePeriod, + "Freeze period not expired" + ); + + require(veto.withdrawableAmount > 0, "No funds to withdraw"); + + uint64 amount = veto.withdrawableAmount; + + emit VetoedFundsWithdrawn(redemptionKey, msg.sender, amount); + + veto.withdrawableAmount = 0; + bank.transferBalance(msg.sender, amount); + } } diff --git a/solidity/test/bridge/RedemptionWatchtower.test.ts b/solidity/test/bridge/RedemptionWatchtower.test.ts index 8336d078c..76133aa92 100644 --- a/solidity/test/bridge/RedemptionWatchtower.test.ts +++ b/solidity/test/bridge/RedemptionWatchtower.test.ts @@ -1067,9 +1067,9 @@ describe("RedemptionWatchtower", () => { it("should update veto state properly", async () => { // Penalty fee is 5% of the redemption amount. const penaltyFee = legacyRedemption.amount.mul(5).div(100) - // The claimable amount left on the watchtower should + // The withdrawable amount left on the watchtower should // be equal to the redemption amount minus the penalty fee. - const claimableAmount = + const withdrawableAmount = legacyRedemption.amount.sub(penaltyFee) expect( @@ -1078,7 +1078,7 @@ describe("RedemptionWatchtower", () => { ) ).to.be.eql([ legacyRedemption.redeemer, - claimableAmount, + withdrawableAmount, // Finalization time is equal to the last block time. await lastBlockTime(), 3, @@ -1158,7 +1158,7 @@ describe("RedemptionWatchtower", () => { ) }) - it("should leave a proper claimable amount and burn the penalty fee", async () => { + it("should leave a proper withdrawable amount and burn the penalty fee", async () => { const currentWatchtowerBalance = await bank.balanceOf( redemptionWatchtower.address ) @@ -1170,7 +1170,7 @@ describe("RedemptionWatchtower", () => { // Penalty fee is 5% of the redemption amount. const penaltyFee = legacyRedemption.amount.mul(5).div(100) - // The claimable amount left on the watchtower should + // The withdrawable amount left on the watchtower should // be equal to the redemption amount minus the penalty fee. expect(difference).to.be.equal( legacyRedemption.amount.sub(penaltyFee) @@ -1422,9 +1422,9 @@ describe("RedemptionWatchtower", () => { it("should update veto state properly", async () => { // Penalty fee is 5% of the redemption amount. const penaltyFee = redemption.amount.mul(5).div(100) - // The claimable amount left on the watchtower should + // The withdrawable amount left on the watchtower should // be equal to the redemption amount minus the penalty fee. - const claimableAmount = redemption.amount.sub(penaltyFee) + const withdrawableAmount = redemption.amount.sub(penaltyFee) expect( await redemptionWatchtower.vetoProposals( @@ -1432,7 +1432,7 @@ describe("RedemptionWatchtower", () => { ) ).to.be.eql([ redemption.redeemer, - claimableAmount, + withdrawableAmount, // Finalization time is equal to the last block time. await lastBlockTime(), 3, @@ -1507,7 +1507,7 @@ describe("RedemptionWatchtower", () => { ) }) - it("should leave a proper claimable amount and burn the penalty fee", async () => { + it("should leave a proper withdrawable amount and burn the penalty fee", async () => { const currentWatchtowerBalance = await bank.balanceOf( redemptionWatchtower.address ) @@ -1519,7 +1519,7 @@ describe("RedemptionWatchtower", () => { // Penalty fee is 5% of the redemption amount. const penaltyFee = redemption.amount.mul(5).div(100) - // The claimable amount left on the watchtower should + // The withdrawable amount left on the watchtower should // be equal to the redemption amount minus the penalty fee. expect(difference).to.be.equal( redemption.amount.sub(penaltyFee) @@ -2309,6 +2309,416 @@ describe("RedemptionWatchtower", () => { }) }) + describe("withdrawVetoedFunds", () => { + let redemption: RedemptionData + let redeemerSigner: SignerWithAddress + + before(async () => { + await createSnapshot() + + await redemptionWatchtower.connect(governance).enableWatchtower( + redemptionWatchtowerManager.address, + guardians.map((g) => g.address) + ) + + const redemptions = await createRedemptionRequests( + SinglePendingRequestedRedemption + ) + // eslint-disable-next-line prefer-destructuring + redemption = redemptions[0] + + redeemerSigner = await impersonateAccount(redemption.redeemer, { + from: governance, + value: 10, + }) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the veto is not finalized", () => { + context("when there are no objections at all", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redeemerSigner) + .withdrawVetoedFunds(redemption.redemptionKey) + ).to.be.revertedWith("Redemption veto not finalized") + }) + }) + + context("when there some objections", () => { + before(async () => { + await createSnapshot() + + // Raise two objections but do not finalize the veto. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redeemerSigner) + .withdrawVetoedFunds(redemption.redemptionKey) + ).to.be.revertedWith("Redemption veto not finalized") + }) + }) + }) + + context( + "when the veto is finalized and the penalty fee is lesser than 100%", + () => { + let withdrawableAmount: BigNumber + + before(async () => { + await createSnapshot() + + // Change the default 100% penalty fee to 5%. + await redemptionWatchtower + .connect(redemptionWatchtowerManager) + .updateWatchtowerParameters( + await redemptionWatchtower.watchtowerLifetime(), + 20, + await redemptionWatchtower.vetoFreezePeriod(), + await redemptionWatchtower.defaultDelay(), + await redemptionWatchtower.levelOneDelay(), + await redemptionWatchtower.levelTwoDelay(), + await redemptionWatchtower.waivedAmountLimit() + ) + + // Veto the redemption. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + await redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Withdrawable amount is the redemption amount minus the 5% penalty fee. + withdrawableAmount = redemption.amount.sub(redemption.amount.div(20)) + expect(withdrawableAmount).to.be.equal( + (await redemptionWatchtower.vetoProposals(redemption.redemptionKey)) + .withdrawableAmount + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the caller is not the redeemer", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(thirdParty) + .withdrawVetoedFunds(redemption.redemptionKey) + ).to.be.revertedWith("Caller is not the redeemer") + }) + }) + + context("when the caller is the redeemer", () => { + context("when the freeze period has not expired", () => { + before(async () => { + await createSnapshot() + + const freezePeriodExpiresAt = + ( + await redemptionWatchtower.vetoProposals( + redemption.redemptionKey + ) + ).finalizedAt + (await redemptionWatchtower.vetoFreezePeriod()) + + // Increase time to one second before the freeze period expires. + // The `withdrawVetoedFunds` transaction should be mined at the moment + // of the freeze period expiration which is one second too early + // to perform withdrawal. + await increaseTime( + freezePeriodExpiresAt - (await lastBlockTime()) - 1 + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redeemerSigner) + .withdrawVetoedFunds(redemption.redemptionKey) + ).to.be.revertedWith("Freeze period not expired") + }) + }) + + context("when the freeze period has expired", () => { + before(async () => { + await createSnapshot() + + const freezePeriodExpiresAt = + ( + await redemptionWatchtower.vetoProposals( + redemption.redemptionKey + ) + ).finalizedAt + (await redemptionWatchtower.vetoFreezePeriod()) + + // Increase time to the moment the freeze period expires. + // The `withdrawVetoedFunds` transaction should be mined exactly one + // second after the freeze period expiration. + await increaseTime( + freezePeriodExpiresAt - (await lastBlockTime()) + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when there are no funds to withdraw", () => { + before(async () => { + await createSnapshot() + + // Withdraw the entire amount from the watchtower. + await redemptionWatchtower + .connect(redeemerSigner) + .withdrawVetoedFunds(redemption.redemptionKey) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redeemerSigner) + .withdrawVetoedFunds(redemption.redemptionKey) + ).to.be.revertedWith("No funds to withdraw") + }) + }) + + context("when there are funds to withdraw", () => { + let tx: ContractTransaction + let initialWatchtowerBalance: BigNumber + let initialRedeemerBalance: BigNumber + + before(async () => { + await createSnapshot() + + initialWatchtowerBalance = await bank.balanceOf( + redemptionWatchtower.address + ) + initialRedeemerBalance = await bank.balanceOf( + redemption.redeemer + ) + + // Withdraw the entire amount from the watchtower. + tx = await redemptionWatchtower + .connect(redeemerSigner) + .withdrawVetoedFunds(redemption.redemptionKey) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should emit VetoedFundsWithdrawn event", async () => { + await expect(tx) + .to.emit(redemptionWatchtower, "VetoedFundsWithdrawn") + .withArgs( + redemption.redemptionKey, + redemption.redeemer, + withdrawableAmount + ) + }) + + it("should set withdrawable amount to zero", async () => { + expect( + ( + await redemptionWatchtower.vetoProposals( + redemption.redemptionKey + ) + ).withdrawableAmount + ).to.be.equal(0) + }) + + it("should transfer the funds to the redeemer", async () => { + const currentWatchtowerBalance = await bank.balanceOf( + redemptionWatchtower.address + ) + const currentRedeemerBalance = await bank.balanceOf( + redemption.redeemer + ) + + // Watchtower's balance decreased. + const watchtowerDifference = initialWatchtowerBalance.sub( + currentWatchtowerBalance + ) + // Redeemer's balance increased. + const redeemerDifference = currentRedeemerBalance.sub( + initialRedeemerBalance + ) + + expect(watchtowerDifference).to.be.equal(withdrawableAmount) + expect(redeemerDifference).to.be.equal(withdrawableAmount) + + // Double-check the right event was emitted. + await expect(tx) + .to.emit(bank, "BalanceTransferred") + .withArgs( + redemptionWatchtower.address, + redemption.redeemer, + withdrawableAmount + ) + }) + }) + }) + }) + } + ) + + context("when the veto is finalized and the penalty fee is 100%", () => { + let withdrawableAmount: BigNumber + + before(async () => { + await createSnapshot() + + // Veto the redemption. + await redemptionWatchtower + .connect(guardians[0]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + await redemptionWatchtower + .connect(guardians[1]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + await redemptionWatchtower + .connect(guardians[2]) + .raiseObjection( + redemption.walletPublicKeyHash, + redemption.redeemerOutputScript + ) + + // Withdrawable amount is 0 as the default penalty fee is 100%. + withdrawableAmount = BigNumber.from(0) + expect(withdrawableAmount).to.be.equal( + (await redemptionWatchtower.vetoProposals(redemption.redemptionKey)) + .withdrawableAmount + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + context("when the caller is not the redeemer", () => { + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(thirdParty) + .withdrawVetoedFunds(redemption.redemptionKey) + ).to.be.revertedWith("Caller is not the redeemer") + }) + }) + + context("when the caller is the redeemer", () => { + context("when the freeze period has not expired", () => { + before(async () => { + await createSnapshot() + + const freezePeriodExpiresAt = + ( + await redemptionWatchtower.vetoProposals( + redemption.redemptionKey + ) + ).finalizedAt + (await redemptionWatchtower.vetoFreezePeriod()) + + // Increase time to one second before the freeze period expires. + // The `withdrawVetoedFunds` transaction should be mined at the moment + // of the freeze period expiration which is one second too early + // to perform withdrawal. + await increaseTime( + freezePeriodExpiresAt - (await lastBlockTime()) - 1 + ) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redeemerSigner) + .withdrawVetoedFunds(redemption.redemptionKey) + ).to.be.revertedWith("Freeze period not expired") + }) + }) + + context("when the freeze period has expired", () => { + before(async () => { + await createSnapshot() + + const freezePeriodExpiresAt = + ( + await redemptionWatchtower.vetoProposals( + redemption.redemptionKey + ) + ).finalizedAt + (await redemptionWatchtower.vetoFreezePeriod()) + + // Increase time to the moment the freeze period expires. + // The `withdrawVetoedFunds` transaction should be mined exactly one + // second after the freeze period expiration. + await increaseTime(freezePeriodExpiresAt - (await lastBlockTime())) + }) + + after(async () => { + await restoreSnapshot() + }) + + it("should revert", async () => { + await expect( + redemptionWatchtower + .connect(redeemerSigner) + .withdrawVetoedFunds(redemption.redemptionKey) + ).to.be.revertedWith("No funds to withdraw") + }) + }) + }) + }) + }) + type RedemptionData = { redemptionKey: string walletPublicKeyHash: string From 5346e0e41ea26bfd2a1bb42190161f8816199195 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Thu, 22 Feb 2024 10:58:29 +0100 Subject: [PATCH 21/23] s/redemptionkey/redemptionKey --- solidity/contracts/bridge/RedemptionWatchtower.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index 80f043e06..98d32861b 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -157,7 +157,7 @@ contract RedemptionWatchtower is OwnableUpgradeable { event Unbanned(address indexed redeemer); event VetoedFundsWithdrawn( - uint256 indexed redemptionkey, + uint256 indexed redemptionKey, address indexed redeemer, uint64 amount ); From 2556c282c82b62114472bc8ff62396eea6e01ec4 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Thu, 29 Feb 2024 10:56:09 +0100 Subject: [PATCH 22/23] Extract required objections count to a constant --- solidity/contracts/bridge/RedemptionWatchtower.sol | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index 98d32861b..dc9c56e69 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -48,6 +48,9 @@ contract RedemptionWatchtower is OwnableUpgradeable { uint8 objectionsCount; } + /// @notice Number of guardian objections required to veto a redemption request. + uint8 public constant REQUIRED_OBJECTIONS_COUNT = 3; + /// @notice Set of redemption guardians. mapping(address => bool) public isGuardian; /// @notice Set of banned redeemer addresses. Banned redeemers cannot @@ -312,10 +315,8 @@ contract RedemptionWatchtower is OwnableUpgradeable { VetoProposal storage veto = vetoProposals[redemptionKey]; - uint8 requiredObjectionsCount = 3; - require( - veto.objectionsCount < requiredObjectionsCount, + veto.objectionsCount < REQUIRED_OBJECTIONS_COUNT, "Redemption request already vetoed" ); @@ -367,7 +368,7 @@ contract RedemptionWatchtower is OwnableUpgradeable { emit ObjectionRaised(redemptionKey, msg.sender); // If there are enough objections, finalize the veto. - if (veto.objectionsCount == requiredObjectionsCount) { + if (veto.objectionsCount == REQUIRED_OBJECTIONS_COUNT) { // Calculate the veto penalty fee that will be deducted from the // final amount that the redeemer can claim after the freeze period. uint64 penaltyFee = vetoPenaltyFeeDivisor > 0 From c6c03a373ac60cd6195b3216da6665e569d1e386 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 5 Mar 2024 16:58:43 +0100 Subject: [PATCH 23/23] Improve documentation comments around redemptions veto --- solidity/contracts/bridge/BridgeState.sol | 5 ++++- solidity/contracts/bridge/Redemption.sol | 17 +++++++++++++++-- .../contracts/bridge/RedemptionWatchtower.sol | 8 ++++++-- .../bridge/WalletProposalValidator.sol | 8 ++++++++ 4 files changed, 33 insertions(+), 5 deletions(-) diff --git a/solidity/contracts/bridge/BridgeState.sol b/solidity/contracts/bridge/BridgeState.sol index 277d8c52d..6e21a4698 100644 --- a/solidity/contracts/bridge/BridgeState.sol +++ b/solidity/contracts/bridge/BridgeState.sol @@ -181,7 +181,10 @@ library BridgeState { // timed out. It is counted from the moment when the redemption // request was created via `requestRedemption` call. Reported // timed out requests are cancelled and locked TBTC is returned - // to the redeemer in full amount. + // to the redeemer in full amount. If a redemption watchtower + // is set, the redemption timeout should be greater than the maximum + // value of the redemption delay that can be enforced by the watchtower. + // Consult `IRedemptionWatchtower.getRedemptionDelay` for more details. uint32 redemptionTimeout; // The amount of stake slashed from each member of a wallet for a // redemption timeout. diff --git a/solidity/contracts/bridge/Redemption.sol b/solidity/contracts/bridge/Redemption.sol index 74ae1106b..8e2c41b65 100644 --- a/solidity/contracts/bridge/Redemption.sol +++ b/solidity/contracts/bridge/Redemption.sol @@ -34,7 +34,15 @@ interface IRedemptionWatchtower { /// 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. + /// @param redeemer The address that requested the redemption and will be + /// able to claim Bank balance if anything goes wrong during the + /// redemption. In the most basic case, when someone redeems their + /// Bitcoin balance from the Bank, `balanceOwner` is the same + /// as `redeemer`. However, when a Vault is redeeming part of its + /// balance for some redeemer address (for example, someone who has + /// earlier deposited into that Vault), `balanceOwner` is the Vault, + /// and `redeemer` is the address for which the vault is redeeming + /// its balance to. /// @return True if the redemption request is safe, false otherwise. /// Specific safety criteria depend on the implementation. function isSafeRedemption( @@ -413,6 +421,8 @@ library Redemption { /// `amount - (amount / redemptionTreasuryFeeDivisor) - redemptionTxMaxFee`. /// Fees values are taken at the moment of request creation. /// @dev Requirements: + /// - If the redemption watchtower is set, the redemption request must + /// be considered safe by the watchtower, /// - Wallet behind `walletPubKeyHash` must be live, /// - `mainUtxo` components must point to the recent main UTXO /// of the given wallet, as currently known on the Ethereum chain, @@ -1169,7 +1179,10 @@ library Redemption { redemption.treasuryFee; // Capture the amount that should be transferred to the - // redemption watchtower. + // redemption watchtower. Use the whole requested amount as a detained + // amount because the treasury fee is deducted in `submitRedemptionProof`. + // Since the redemption did not happen, the treasury fee was not + // deducted and the whole requested amount should be detained. uint64 detainedAmount = redemption.requestedAmount; // Delete the redemption request from the pending redemptions diff --git a/solidity/contracts/bridge/RedemptionWatchtower.sol b/solidity/contracts/bridge/RedemptionWatchtower.sol index dc9c56e69..dd42fbddb 100644 --- a/solidity/contracts/bridge/RedemptionWatchtower.sol +++ b/solidity/contracts/bridge/RedemptionWatchtower.sol @@ -328,6 +328,9 @@ contract RedemptionWatchtower is OwnableUpgradeable { Redemption.RedemptionRequest memory redemption = bridge .pendingRedemptions(redemptionKey); + // This also handles the case when the redemption was already processed + // given we delete successful ones from the `pendingRedemptions` mapping + // of the `Bridge` state. require( redemption.requestedAt != 0, "Redemption request does not exist" @@ -343,8 +346,9 @@ contract RedemptionWatchtower is OwnableUpgradeable { if (redemption.requestedAt >= watchtowerEnabledAt) { require( // Use < instead of <= to avoid a theoretical edge case - // where the delay is 0 (veto disabled) but three objections - // are raised in the same block as the redemption request. + // where the delay is 0 (watchtower disabled OR amount below + // the veto threshold) but three objections are raised in the + // same block as the redemption request. /* solhint-disable-next-line not-rely-on-time */ block.timestamp < redemption.requestedAt + diff --git a/solidity/contracts/bridge/WalletProposalValidator.sol b/solidity/contracts/bridge/WalletProposalValidator.sol index 683027a67..d2e5305b1 100644 --- a/solidity/contracts/bridge/WalletProposalValidator.sol +++ b/solidity/contracts/bridge/WalletProposalValidator.sol @@ -620,6 +620,14 @@ contract WalletProposalValidator { .getRedemptionDelay(redemptionKey); // If the delay is greater than the usual minimum age, use it. // This way both the min age and the watchtower delay are preserved. + // + // We do not need to bother about last-minute objections issued + // by the watchtower. Objections can be issued up to one second + // before the min age is achieved while this validation will + // pass only one second after the min age is achieved. Even if + // a single objection stays longer in the mempool, this won't + // be a problem for `Bridge.submitRedemptionProof` which ignores + // single objections as long as the veto threshold is not reached. if (delay > minAge) { minAge = delay; }