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

merged 4 commits into from
Dec 21, 2023

Conversation

tomaszslabon
Copy link
Contributor

@tomaszslabon tomaszslabon commented Dec 13, 2023 •
edited
Loading

#Refs: keep-network/keep-core#3733.

This PR adds moving funds proposal validation to the wallet proposal validator smart contract.
The validation performs several checks:

  • the source wallet's state must be MovingFunds,
  • the source wallet's commitment hash must be submitted,
  • the target wallets from the proposal must match the submitted commitment hash,
  • the proposed fee must be greater than zero,
  • the transaction proposed fee must not exceed the maximum fee.

@tomaszslabon tomaszslabon self-assigned this Dec 13, 2023
@tomaszslabon tomaszslabon added the ⛓️ solidity Solidity contracts label Dec 13, 2023
@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7193826152 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7194743765 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7194802143 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7194942620 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7195153227 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7195290897 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7198643157 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7198688491 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7208309734 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7209811020 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7210208652 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7211457982 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7213310007 check.

@tomaszslabon tomaszslabon marked this pull request as ready for review December 14, 2023 19:04
lukasz-zimnoch
lukasz-zimnoch reviewed Dec 19, 2023
View reviewed changes
solidity/contracts/bridge/WalletProposalValidator.sol Outdated
/// zero,
/// - The proposed moving funds transaction fee must not exceed the
/// maximum total fee allowed for moving funds.
function validateMovingFundsProposal(
Copy link
Member

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

Choose a reason for hiding this comment

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

This function can be simplified as we can leverage the fact that on-chain commitment submission is also a kind of validation. Looks like it is enough that validateMovingFundsProposal makes sure that:

  • The source wallet's state is MovingFunds
  • The source wallet's commitment hash is submitted
  • The target wallets from the proposal match the submitted commitment hash (same wallets and same order)
  • The proposed fee is greater than zero
  • The proposed fee does not exceed the maximum fee

In that model, the coordination leader responsible for generating a moving funds proposal can do the following:

  1. Gather all data necessary to assemble a moving funds proposal
  2. Submit the commitment (or check if it is already submitted). Before doing that, the leader can make sure the commitment can be submitted using a non-mutating call. This can avoid burning gas if the wallet's state does not allow for commitment submission.
  3. Wait for the commitment transaction to be mined
  4. Call validateMovingFundsProposal to make sure the proposal is actually valid and can be safely broadcasted
  5. Return the proposal

On the other hand, coordination followers receiving a moving funds proposal should:

  1. Call validateMovingFundsProposal to make sure the proposal is actually submitted and valid
  2. Wait several blocks to confirm proposal validity (32 blocks?). This is needed to avoid corner cases where the commitment is submitted but not included in the canonical chain due to a reorg and another front-running transaction that affects commitment validity.
  3. Call validateMovingFundsProposal again to ensure the commitment has not been changed during the idleness period
  4. Assemble, sign, and broadcast moving funds transaction on Bitcoin

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 3f7d6b3.

Copy link
Member

Choose a reason for hiding this comment

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

Leaving it open for future reference!

@lukasz-zimnoch lukasz-zimnoch mentioned this pull request Dec 19, 2023
Closed
@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7262967420 check.

@tomaszslabon tomaszslabon force-pushed the update-proposal-validator branch 2 times, most recently from 7a889db to 937fbc7 Compare December 19, 2023 14:57
@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7263544294 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7263559875 check.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7263642167 check.

@tomaszslabon tomaszslabon mentioned this pull request Dec 20, 2023
Merged
lukasz-zimnoch
lukasz-zimnoch approved these changes Dec 21, 2023
View reviewed changes
solidity/contracts/bridge/WalletProposalValidator.sol
/// 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.

@github-actions GitHub Actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/tbtc-v2/actions/runs/7290592585 check.

@lukasz-zimnoch lukasz-zimnoch merged commit 05ec6c1 into main Dec 21, 2023
36 checks passed
@lukasz-zimnoch lukasz-zimnoch deleted the update-proposal-validator branch December 21, 2023 16:19
@tomaszslabon tomaszslabon mentioned this pull request Dec 21, 2023
Merged
lukasz-zimnoch added a commit that referenced this pull request Dec 22, 2023
@lukasz-zimnoch
Added source wallet BTC balance check (#765)
fc29069 
Refs: keep-network/keep-core#3733

This PR is a follow-up to
#757.
The following checks were added:
- source wallet must be positive,
- source wallet BTC balance must be equal to or greater than the
`movingFundsDustThreshold` parameter.
lukasz-zimnoch added a commit that referenced this pull request Jan 9, 2024
@lukasz-zimnoch
Added validation of moved funds sweep proposal (#762)
6709d4e 
#Refs: keep-network/keep-core#3733.
#Depends on: #757.

This PR adds moved funds sweep proposal validation to the wallet
proposal validator smart contract.
@lukasz-zimnoch lukasz-zimnoch mentioned this pull request Feb 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasz-zimnoch lukasz-zimnoch lukasz-zimnoch approved these changes

Assignees

@tomaszslabon tomaszslabon

Labels
⛓️ solidity Solidity contracts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tomaszslabon @lukasz-zimnoch