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.

Sign up for GitHub

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

Jump to bottom

Added validation of moving funds proposal #757

Merged
merged 4 commits into from
Dec 21, 2023
Merged

Added validation of moving funds proposal #757

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a021e8
Added validation of moving funds proposal
tomaszslabon Dec 13, 2023
6b58e86
Update unit tests
tomaszslabon Dec 14, 2023
3f7d6b3
Simplified moving funds proposal validation
tomaszslabon Dec 19, 2023
8983a64
Merge branch 'main' into update-proposal-validator
lukasz-zimnoch Dec 21, 2023
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
75 changes: 75 additions & 0 deletions solidity/contracts/bridge/WalletProposalValidator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,16 @@ contract WalletProposalValidator {
uint256 redemptionTxFee;
}

/// @notice Helper structure representing a moving funds proposal.
struct MovingFundsProposal {
// 20-byte public key hash of the source wallet.
bytes20 walletPubKeyHash;
// List of 20-byte public key hashes of target wallets.
bytes20[] targetWallets;
// Proposed BTC fee for the entire transaction.
uint256 movingFundsTxFee;
}

/// @notice Helper structure representing a heartbeat proposal.
struct HeartbeatProposal {
// 20-byte public key hash of the target wallet.
Expand Down Expand Up @@ -587,6 +597,71 @@ contract WalletProposalValidator {
return true;
}

/// @notice View function encapsulating the main rules of a valid moving
/// funds proposal. This function is meant to facilitate the
/// off-chain validation of the incoming proposals. Thanks to it,
/// most of the work can be done using a single readonly contract
/// call.
/// @param proposal The moving funds proposal to validate.
/// @return True if the proposal is valid. Reverts otherwise.
/// @dev Notice that this function is meant to be invoked after the moving
/// funds commitment has already been submitted. This function skips
/// some checks related to the moving funds procedure as they were
/// already checked on the commitment submission.
/// Requirements:
/// - The source wallet must be in the MovingFunds state,
/// - The target wallets commitment must be submitted,
/// - The target wallets commitment hash must match the target wallets
/// from the proposal,
/// - The proposed moving funds transaction fee must be greater than
/// zero,
/// - The proposed moving funds transaction fee must not exceed the
/// maximum total fee allowed for moving funds.
function validateMovingFundsProposal(MovingFundsProposal calldata proposal)
Copy link
Member

@lukasz-zimnoch lukasz-zimnoch Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think there is an additional thing that should be checked in this function. Namely:

require(
    walletBtcBalance >= movingFundsDustThreshold,
    "Wallet BTC balance must be equal or above the moving funds dust threshold"
);

This is because there is the notifyMovingFundsBelowDust path for wallets whose balance is below the dust. If we don't check that here, the following may happen:

  1. The leader submits a commitment
  2. The leader creates a proposal
  3. The leader validates the proposal using validateMovingFundsProposal
  4. The leader broadcasts the proposal
  5. Followers receive the proposal and validate it using validateMovingFundsProposal
  6. Followers start the action that leads to a BTC moving funds transaction
  7. A third party calls notifyMovingFundsBelowDust that moves the wallet to the Closing state
  8. Submitting the SPV through submitMovingFundsProof is no longer possible as the wallet must be in the MovingFunds state
  9. A third party accuses the wallet of committing fraud. The wallet cannot defeat the challenge as the SPV proof cannot be submitted so the main UTXO was spent illegally

The leader will not broadcast the proposal if we add the aforementioned require to validateMovingFundsProposal. It can still submit the commitment (because that happens before proposal validation, see #757 (comment)) but that's not a problem. On the other hand, if a leader broadcasts the proposal regardless, the followers will reject it due to this require.

Please verify my understanding. We can merge this PR to not block it and add this check in a follow-up if you find the above scenario correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in db47dbb.
This check indeed seems to be necessary.

external
view
returns (bool)
{
Wallets.Wallet memory sourceWallet = bridge.wallets(
proposal.walletPubKeyHash
);

// Make sure the source wallet is in MovingFunds state.
require(
sourceWallet.state == Wallets.WalletState.MovingFunds,
"Source wallet is not in MovingFunds state"
);

// Make sure the moving funds commitment has been submitted and
// the commitment hash matches the target wallets from the proposal.
require(
sourceWallet.movingFundsTargetWalletsCommitmentHash != bytes32(0),
"Target wallets commitment is not submitted"
);

require(
sourceWallet.movingFundsTargetWalletsCommitmentHash ==
keccak256(abi.encodePacked(proposal.targetWallets)),
"Target wallets do not match target wallets commitment hash"
);

// Make sure the proposed fee is valid.
(uint64 movingFundsTxMaxTotalFee, , , , , , , , , , ) = bridge
.movingFundsParameters();

require(
proposal.movingFundsTxFee > 0,
"Proposed transaction fee cannot be zero"
);

require(
proposal.movingFundsTxFee <= movingFundsTxMaxTotalFee,
"Proposed transaction fee is too high"
);

return true;
}

/// @notice View function encapsulating the main rules of a valid heartbeat
/// proposal. This function is meant to facilitate the off-chain
/// validation of the incoming proposals. Thanks to it, most
Expand Down
238 changes: 238 additions & 0 deletions solidity/test/bridge/WalletProposalValidator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { FakeContract, smock } from "@defi-wonderland/smock"
import { BigNumber, BigNumberish, BytesLike } from "ethers"
import type { Bridge, WalletProposalValidator } from "../../typechain"
import { walletState } from "../fixtures"
import { NO_MAIN_UTXO } from "../data/deposit-sweep"

chai.use(smock.matchers)

Expand Down Expand Up @@ -1894,6 +1895,243 @@ describe("WalletProposalValidator", () => {
})
})

describe("validateMovingFundsProposal", () => {
const walletPubKeyHash = "0x7ac2d9378a1c47e589dfb8095ca95ed2140d2726"
const targetWallets = [
"0x84a70187011e156686788e0a2bc50944a4721e83",
"0xf64a45c07e3778b8ce58cb0058477c821c543aad",
"0xcaea95433d9bfa80bb8dc8819a48e2a9aa96147c",
]
// Hash calculated from the above target wallets.
const targetWalletsHash =
"0x16311d424d513a1743fbc9c0e4fea5b70eddefd15f54613503e5cdfab24f8877"
const movingFundsTxMaxTotalFee = 20000

before(async () => {
await createSnapshot()

bridge.movingFundsParameters.returns([
movingFundsTxMaxTotalFee,
0,
0,
0,
0,
0,
0,
0,
0,
0,
0,
])
})

after(async () => {
bridge.movingFundsParameters.reset()

await restoreSnapshot()
})

context("when wallet's state is not MovingFunds", () => {
const testData = [
{
testName: "when wallet state is Unknown",
walletState: walletState.Unknown,
},
{
testName: "when wallet state is Live",
walletState: walletState.Live,
},
{
testName: "when wallet state is Closing",
walletState: walletState.Closing,
},
{
testName: "when wallet state is Closed",
walletState: walletState.Closed,
},
{
testName: "when wallet state is Terminated",
walletState: walletState.Terminated,
},
]

testData.forEach((test) => {
context(test.testName, () => {
before(async () => {
await createSnapshot()

bridge.wallets.whenCalledWith(walletPubKeyHash).returns({
ecdsaWalletID: HashZero,
mainUtxoHash: HashZero,
pendingRedemptionsValue: 0,
createdAt: 0,
movingFundsRequestedAt: 0,
closingStartedAt: 0,
pendingMovedFundsSweepRequestsCount: 0,
state: test.walletState,
movingFundsTargetWalletsCommitmentHash: HashZero,
})
})

after(async () => {
bridge.wallets.reset()

await restoreSnapshot()
})

it("should revert", async () => {
await expect(
walletProposalValidator.validateMovingFundsProposal({
walletPubKeyHash,
movingFundsTxFee: 0,
targetWallets: [],
})
).to.be.revertedWith("Source wallet is not in MovingFunds state")
})
})
})
})

context("when wallet's state is MovingFunds", () => {
context("when moving funds commitment has not been submitted", () => {
before(async () => {
await createSnapshot()

bridge.wallets.whenCalledWith(walletPubKeyHash).returns({
ecdsaWalletID: HashZero,
mainUtxoHash: HashZero,
pendingRedemptionsValue: 0,
createdAt: 0,
movingFundsRequestedAt: 0,
closingStartedAt: 0,
pendingMovedFundsSweepRequestsCount: 0,
state: walletState.MovingFunds,
// Indicate the commitment has not been submitted.
movingFundsTargetWalletsCommitmentHash: HashZero,
})
})

after(async () => {
bridge.wallets.reset()

await restoreSnapshot()
})

it("should revert", async () => {
await expect(
walletProposalValidator.validateMovingFundsProposal({
walletPubKeyHash,
movingFundsTxFee: 0,
targetWallets: [],
})
).to.be.revertedWith("Target wallets commitment is not submitted")
})
})

context("when moving funds commitment has been submitted", () => {
context("when commitment hash does not match target wallets", () => {
before(async () => {
await createSnapshot()

bridge.wallets.whenCalledWith(walletPubKeyHash).returns({
ecdsaWalletID: HashZero,
mainUtxoHash: HashZero,
pendingRedemptionsValue: 0,
createdAt: 0,
movingFundsRequestedAt: 0,
closingStartedAt: 0,
pendingMovedFundsSweepRequestsCount: 0,
state: walletState.MovingFunds,
// Set a hash that does not match the target wallets.
movingFundsTargetWalletsCommitmentHash:
"0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
})
})

after(async () => {
bridge.wallets.reset()

await restoreSnapshot()
})

it("should revert", async () => {
await expect(
walletProposalValidator.validateMovingFundsProposal({
walletPubKeyHash,
movingFundsTxFee: 0,
targetWallets,
})
).to.be.revertedWith(
"Target wallets do not match target wallets commitment hash"
)
})
})

context("when commitment hash matches target wallets", () => {
before(async () => {
await createSnapshot()

bridge.wallets.whenCalledWith(walletPubKeyHash).returns({
ecdsaWalletID: HashZero,
mainUtxoHash: HashZero,
pendingRedemptionsValue: 0,
createdAt: 0,
movingFundsRequestedAt: 0,
closingStartedAt: 0,
pendingMovedFundsSweepRequestsCount: 0,
state: walletState.MovingFunds,
// Set the correct hash.
movingFundsTargetWalletsCommitmentHash: targetWalletsHash,
})
})

after(async () => {
bridge.wallets.reset()

await restoreSnapshot()
})

context("when transaction fee is zero", () => {
it("should revert", async () => {
await expect(
walletProposalValidator.validateMovingFundsProposal({
walletPubKeyHash,
movingFundsTxFee: 0,
targetWallets,
})
).to.be.revertedWith("Proposed transaction fee cannot be zero")
})
})

context("when transaction fee is too high", () => {
it("should revert", async () => {
await expect(
walletProposalValidator.validateMovingFundsProposal({
walletPubKeyHash,
movingFundsTxFee: movingFundsTxMaxTotalFee + 1,
targetWallets,
})
).to.be.revertedWith("Proposed transaction fee is too high")
})
})

context("when transaction fee is valid", () => {
it("should pass validation", async () => {
const result =
await walletProposalValidator.validateMovingFundsProposal({
walletPubKeyHash,
movingFundsTxFee: movingFundsTxMaxTotalFee,
targetWallets,
})
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
expect(result).to.be.true
})
})
})
})
})
})

describe("validateHeartbeatProposal", () => {
context("when message is not valid", () => {
it("should revert", async () => {
Expand Down
Loading