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

Added validation of moved funds sweep proposal #762

Merged
merged 4 commits into from
Jan 9, 2024
Merged

Added validation of moved funds sweep proposal #762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a578602
Added validation of moved funds sweep proposal
tomaszslabon Dec 20, 2023
2f2f3ca
Merge branch 'main' into update-proposal-validator-1
tomaszslabon Dec 22, 2023
b6fb047
Added docstring
tomaszslabon Dec 22, 2023
d649d4f
Added unit tests
tomaszslabon Dec 29, 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
89 changes: 89 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 @@ -22,6 +22,7 @@ import "./BitcoinTx.sol";
import "./Bridge.sol";
import "./Deposit.sol";
import "./Redemption.sol";
import "./MovingFunds.sol";
import "./Wallets.sol";

/// @title Wallet proposal validator.
Expand Down Expand Up @@ -104,6 +105,20 @@ contract WalletProposalValidator {
uint256 movingFundsTxFee;
}

/// @notice Helper structure representing a moved funds sweep proposal.
struct MovedFundsSweepProposal {
// 20-byte public key hash of the wallet.
bytes20 walletPubKeyHash;
// 32-byte hash of the moving funds transaction that caused the sweep
// request to be created.
bytes32 movingFundsTxHash;
// Index of the moving funds transaction output that is subject of the
// sweep request.
uint32 movingFundsTxOutputIndex;
// Proposed BTC fee for the entire transaction.
uint256 movedFundsSweepTxFee;
}

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

/// @notice View function encapsulating the main rules of a valid moved
/// funds sweep 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 moved funds sweep proposal to validate.
/// @return True if the proposal is valid. Reverts otherwise.
/// @dev Requirements:
/// - The source wallet must be in the Live or MovingFunds state,
/// - The moved funds sweep request identified by the proposed
/// transaction hash and output index must be in the Pending state,
/// - The transaction hash and output index from the proposal must
/// identify a moved funds sweep request in the Pending state,
/// - The transaction hash and output index from the proposal must
/// identify a moved funds sweep request that belongs to the wallet,
/// - The proposed moved funds sweep transaction fee must be greater
/// than zero,
/// - The proposed moved funds sweep transaction fee must not exceed
/// the maximum total fee allowed for moved funds sweep.
function validateMovedFundsSweepProposal(
MovedFundsSweepProposal calldata proposal
) external view returns (bool) {
Wallets.Wallet memory wallet = bridge.wallets(
proposal.walletPubKeyHash
);

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

// Make sure the moved funds sweep request is valid.
uint256 sweepRequestKeyUint = uint256(
keccak256(
abi.encodePacked(
proposal.movingFundsTxHash,
proposal.movingFundsTxOutputIndex
)
)
);

MovingFunds.MovedFundsSweepRequest memory sweepRequest = bridge
.movedFundsSweepRequests(sweepRequestKeyUint);

require(
sweepRequest.state ==
MovingFunds.MovedFundsSweepRequestState.Pending,
"Sweep request is not in Pending state"
);

require(
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.

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

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

require(
proposal.movedFundsSweepTxFee <= movedFundsSweepTxMaxTotalFee,
"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
Loading
Loading