Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redemptions veto mechanism #788

Merged
merged 24 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b42f031
Introduce the `RedemptionWatchtower` contract
lukasz-zimnoch Feb 19, 2024
c97e873
Turn on the watchtower and manage guardians
lukasz-zimnoch Feb 19, 2024
941cf9f
Add `RedemptionWatchtower` state
lukasz-zimnoch Feb 19, 2024
9fe9d9a
Objection mechanism
lukasz-zimnoch Feb 19, 2024
6c5dc8d
Better observability for objections related to legacy redemptions
lukasz-zimnoch Feb 20, 2024
68d5efd
Expose a function allowing to update watchtower parameters
lukasz-zimnoch Feb 20, 2024
6aad28a
Change order of checks in `raiseObjection` function
lukasz-zimnoch Feb 20, 2024
45d3afb
Unit tests for `raiseObjection` function
lukasz-zimnoch Feb 20, 2024
4556003
Unit tests for `updateWatchtowerParameters` function
lukasz-zimnoch Feb 20, 2024
912f96a
Unit tests for `notifyRedemptionVeto` function
lukasz-zimnoch Feb 20, 2024
c57f4f1
Add watchtower check upon redemption request
lukasz-zimnoch Feb 20, 2024
3ab6ee5
Address Slither warning about variable shadowing
lukasz-zimnoch Feb 21, 2024
a62b2e9
Respect the watchtower's redemption delay within `WalletProposalValid…
lukasz-zimnoch Feb 21, 2024
8d7d51b
Expose the `disableWatchtower` function
lukasz-zimnoch Feb 21, 2024
743e9c7
Add explanation about `setRedemptionWatchtower` function
lukasz-zimnoch Feb 21, 2024
6b0a76a
Unit test covering the case when manager is not set and `addGuardian`…
lukasz-zimnoch Feb 21, 2024
584c4bb
Unit test for `disableWatchtower` function
lukasz-zimnoch Feb 21, 2024
d3cc2e8
Suppress Slither's false positives
lukasz-zimnoch Feb 21, 2024
7425eba
Expose `unban` function
lukasz-zimnoch Feb 21, 2024
6a2932d
Expose `withdrawVetoedFunds` function
lukasz-zimnoch Feb 21, 2024
5346e0e
s/redemptionkey/redemptionKey
lukasz-zimnoch Feb 22, 2024
2556c28
Extract required objections count to a constant
lukasz-zimnoch Feb 29, 2024
b6aef91
Merge branch 'main' into optimistic-redemption-1
lukasz-zimnoch Feb 29, 2024
c6c03a3
Improve documentation comments around redemptions veto
lukasz-zimnoch Mar 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions solidity/contracts/bridge/Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ contract Bridge is

event TreasuryUpdated(address treasury);

event RedemptionWatchtowerSet(address redemptionWatchtower);

modifier onlySpvMaintainer() {
require(
self.isSpvMaintainer[msg.sender],
Expand Down Expand Up @@ -1944,4 +1946,48 @@ 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not allow the governance to update the watchtower address? I see it is deployed as a proxy but still... We would avoid upgrading the Bridge contract in case the watchtower address needs to be replaced.

Copy link
Member Author

@lukasz-zimnoch lukasz-zimnoch Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done on purpose to provide a kind of social contract regarding how the mechanism works. We do not expect to change anything in the mechanism itself. Any security upgrade would likely require a Bridge upgrade anyway. Additional argument is Bridge contract size. We do not add anything that is not a must have now.

/// - Redemption watchtower address must not be 0x0.
function setRedemptionWatchtower(address redemptionWatchtower)
external
onlyGovernance
{
// The internal function is defined in the `BridgeState` library.
self.setRedemptionWatchtower(redemptionWatchtower);
lukasz-zimnoch marked this conversation as resolved.
Show resolved Hide resolved
}

/// @return Address of the redemption watchtower.
function getRedemptionWatchtower() 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);
}
}
16 changes: 16 additions & 0 deletions solidity/contracts/bridge/BridgeGovernance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
30 changes: 29 additions & 1 deletion solidity/contracts/bridge/BridgeState.sol
Original file line number Diff line number Diff line change
Expand Up @@ -314,14 +314,17 @@ 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
// planned upgrades of the Bridge contract. If more entires are added to
// 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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
104 changes: 104 additions & 0 deletions solidity/contracts/bridge/Redemption.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,37 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little lost in what is the difference between those two. This will probably get clear as I move forward with the review of the rest of the code but could be an indicator we should improve the docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah: I'd love to copy-paste the explanation of the redeemer from the requestRedemption function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed as part of c6c03a3

/// @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 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
/// validation and to the moving funds transaction proof validation.
library OutboundTx {
Expand Down Expand Up @@ -402,6 +433,19 @@ library Redemption {
bytes memory redeemerOutputScript,
uint64 amount
) internal {
if (self.redemptionWatchtower != address(0)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add this check to the @dev Requirements section?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed as part of c6c03a3

require(
IRedemptionWatchtower(self.redemptionWatchtower)
.isSafeRedemption(
walletPubKeyHash,
redeemerOutputScript,
balanceOwner,
redeemer
),
"Redemption request rejected by the watchtower"
);
}

Wallets.Wallet storage wallet = self.registeredWallets[
walletPubKeyHash
];
Expand Down Expand Up @@ -1075,4 +1119,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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm: we 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 we want to detain everything. Is this a correct understanding? Could be worth dropping a small comment here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is correct. I will drop a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed as part of c6c03a3


// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of the Bank! Redemption Watchtower has a bank balance, I like it very much.

}
}
Loading
Loading