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

Added validation of moved funds sweep proposal #762

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

tomaszslabon
Copy link
Contributor

@tomaszslabon tomaszslabon commented Dec 20, 2023

#Refs: keep-network/keep-core#3733.
#Depends on: #757.

This PR adds moved funds sweep proposal validation to the wallet proposal validator smart contract.

@tomaszslabon tomaszslabon self-assigned this Dec 20, 2023
@tomaszslabon tomaszslabon added the ⛓️ solidity Solidity contracts label Dec 20, 2023
@tomaszslabon tomaszslabon changed the base branch from main to update-proposal-validator December 20, 2023 13:50
Copy link

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

@tomaszslabon tomaszslabon force-pushed the update-proposal-validator-1 branch from c9d4587 to a578602 Compare December 21, 2023 11:04
Copy link

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

Base automatically changed from update-proposal-validator to main December 21, 2023 16:19
Copy link

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

sweepRequest.walletPubKeyHash == proposal.walletPubKeyHash,
"Sweep request does not belong to the wallet"
);

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether we need a kind of safety margin that would prevent handling moved funds sweep requests that are close to reaching their timeout. I mean a similar margin as we use for redemptions (REDEMPTION_REQUEST_TIMEOUT_SAFETY_MARGIN).

For redemptions, the goal of the safety margin is to prevent the case where the redeemer reports a timeout, gets their TBTC back, and additionally receives BTC from a belated wallet.

For moved funds sweep, a safety margin would prevent the case where the sweep request timeout is notified but the belated wallet would produce an unprovable Bitcoin transaction right after. The wallet would be anyway terminated and slashed upon timeout notification. A terminated wallet cannot be accused of fraud so, it wouldn't incur any negative consequences for producing an unprovable Bitcoin transaction. Moreover, if the sweep timeout would be a result of a software error rather than an explicit malice, such a wallet may try to recover to unlock funds. Having all funds under one UTXO would be very helpful in such a case.

That said, I think the safety margin is not needed for this function. Can you double-check and post your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also think a timeout margin is unnecessary in case of moved funds sweep validation.
When a wallet performs a moved funds sweep transaction, it sends the funds to itself.
Therefore It shouldn't be a problem - the funds will end up in the wallet address, but as a single UTXO.
As you mentioned, the wallet will be in the Terminated state, so it won't be accused of a fraud even though it won't be able to submit the transaction proof.

Copy link

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

Copy link

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

@tomaszslabon tomaszslabon marked this pull request as ready for review January 8, 2024 13:28
@lukasz-zimnoch lukasz-zimnoch merged commit 6709d4e into main Jan 9, 2024
38 checks passed
@lukasz-zimnoch lukasz-zimnoch deleted the update-proposal-validator-1 branch January 9, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⛓️ solidity Solidity contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants