Adjust TBTC Transfer Amount to Include Fee and Update Sepolia Deployment

[lrsaturnino]

Summary:

This Pull Request includes the following updates:

- Adjust TBTC Transfer Amount to Include Fee for L2 Deposit Owner

The Threshold DAO has decided to set depositTxMaxFee to zero. Instead of changing the deposit parameters, we have adjusted the amount of TBTC transferred to the L2 deposit owner by adding back the fee amount.
This change is implemented in the contracts inheriting from AbstractTBTCDepositor, without modifying the abstract contract itself. Specifically, depositTxMaxFee is added to the tbtcAmount value returned by _finalizeDeposit.

- Update BaseL1BitcoinDepositor Deployment on Sepolia Network

Deployed the updated BaseL1BitcoinDepositor contract to the Sepolia testnet. This deployment includes the changes where the TBTC transfer amount now accounts for the fee. Ensured compatibility with existing contracts implementing AbstractTBTCDepositor.

Rationale: Align the TBTC transfer mechanism with the Threshold DAO's decision to set the deposit transaction maximum fee to zero, enhancing consistency across the contract implementations.

Implementation: Made minimal changes by adjusting only the inheriting contracts and leaving the abstract contract unchanged, promoting code reusability and maintainability.

[Shadowfiend]

What's the reasoning for not changing the deposit parameters directly? That's governable and doesn't require a contract deployment, right?

[lrsaturnino]

@Shadowfiend

Thanks for your question. While adjusting the depositTxMaxFee parameter directly is governable and doesn't require a contract redeployment, lowering this parameter isn't a viable solution due to the following reasons:

Risk of Stalled Wallet Sweeps: The depositTxMaxFee acts as a cap on the maximum Bitcoin network fee that wallets can use when sweeping deposits. If we lower this cap to reduce the user cost, we risk situations where the actual network fees exceed our cap—especially during periods of high network congestion. This would cause wallets to hold off on sweeping deposits, as they could be accused of fraud and slashed by the protocol for exceeding the fee limit. We've already experienced sweeps stalling during fee surges, like the post-halving fees in April, even with the current depositTxMaxFee value.

Protocol Security Concerns: The fee cap is a crucial part of our security model. It protects against malicious wallets that might otherwise burn user deposits by paying exorbitant fees to miners. Reducing the depositTxMaxFee would weaken this safeguard, potentially exposing the protocol to new attack vectors.

User Experience Impact: Adjusting the parameter downward could lead to unpredictable delays for users, as their deposits might not be swept promptly when network fees are high. This would degrade the overall user experience, which we're striving to improve.

Given these considerations, the proposed change is to remove the subtraction of depositTxMaxFee from the user's minted TBTC amount in the L1BitcoinDepositor contract. Instead, we plan to have the DAO ensure that the L1BitcoinDepositor contract always has enough TBTC liquidity to cover any discrepancies caused by network fee variances.

This approach:

1. Eliminates the immediate cost to users: Users won't have the maximum possible fee deducted upfront, which currently leads to significant leftovers and overcharging.
2. Maintains Protocol Security: We keep the depositTxMaxFee parameter at a level that ensures wallets operate within safe limits.
3. Avoids the Need for Contract Redeployment: By making this change in the contracts implementing AbstractTBTCDepositor rather than in the abstract contract itself, we minimize the scope of changes and avoid affecting other parts of the system.

[somaweb3]

@Shadowfiend

Thanks for your question. While adjusting the depositTxMaxFee parameter directly is governable and doesn't require a contract redeployment, lowering this parameter isn't a viable solution due to the following reasons:

Risk of Stalled Wallet Sweeps: The depositTxMaxFee acts as a cap on the maximum Bitcoin network fee that wallets can use when sweeping deposits. If we lower this cap to reduce the user cost, we risk situations where the actual network fees exceed our cap—especially during periods of high network congestion. This would cause wallets to hold off on sweeping deposits, as they could be accused of fraud and slashed by the protocol for exceeding the fee limit. We've already experienced sweeps stalling during fee surges, like the post-halving fees in April, even with the current depositTxMaxFee value.

Protocol Security Concerns: The fee cap is a crucial part of our security model. It protects against malicious wallets that might otherwise burn user deposits by paying exorbitant fees to miners. Reducing the depositTxMaxFee would weaken this safeguard, potentially exposing the protocol to new attack vectors.

User Experience Impact: Adjusting the parameter downward could lead to unpredictable delays for users, as their deposits might not be swept promptly when network fees are high. This would degrade the overall user experience, which we're striving to improve.

Given these considerations, the proposed change is to remove the subtraction of depositTxMaxFee from the user's minted TBTC amount in the L1BitcoinDepositor contract. Instead, we plan to have the DAO ensure that the L1BitcoinDepositor contract always has enough TBTC liquidity to cover any discrepancies caused by network fee variances.

This approach:

1. Eliminates the immediate cost to users: Users won't have the maximum possible fee deducted upfront, which currently leads to significant leftovers and overcharging.
2. Maintains Protocol Security: We keep the depositTxMaxFee parameter at a level that ensures wallets operate within safe limits.
3. Avoids the Need for Contract Redeployment: By making this change in the contracts implementing AbstractTBTCDepositor rather than in the abstract contract itself, we minimize the scope of changes and avoid affecting other parts of the system.

@Shadowfiend does this explanation satisfactorily address your concern/suggestion?

[Shadowfiend]

Sorry for the late response here—so the intent here is to only deploy these changes to new L2s, yeah? That makes sense.

Given that this changes the underlying code from what existing L2s have, without considering the process of upgrading these L2s, should we create a copy of the depositor contract with a different name, or preserve the existing one with the old name? Not sure how we've approached this kind of change in the past.

[lukasz-zimnoch]

Sorry for the late response here—so the intent here is to only deploy these changes to new L2s, yeah? That makes sense.

Given that this changes the underlying code from what existing L2s have, without considering the process of upgrading these L2s, should we create a copy of the depositor contract with a different name, or preserve the existing one with the old name? Not sure how we've approached this kind of change in the past.

The upgrade process here should be as follows:

1. Introduce changes to the existing Solidity contracts directly.
2. Deploy the new implementation and point the transparent proxy to the new implementation.
3. Update the artifact in cross-chain/base/deployments/sepolia/BaseL1BitcoinDepositor.json
4. Update the artifact in the SDK
5. Ideally, release a new tag and npm package for the SDK

I would probably not copy the contract but maybe introduce a feature flag allowing to enable/disable the reimbursement of the tx fee.

[lrsaturnino]

After a recent conversation, @lukasz-zimnoch suggested a feature flag implementation allowing the contract owner to switch between charging or refunding the txMaxFee from the tBTC amount minted.

Summary of the changes:

• New Storage Variable
  Introduces bool public reimburseTxFee; which controls whether the txMaxFee is reimbursed or deducted:

  • true => The deposit transaction max fee is added to tbtcAmount (the user is not charged a fee).
  • false => The deposit transaction max fee is subtracted from tbtcAmount (original behavior).

• New Admin Function
  Adds setReimburseTxFee(bool _reimburseTxFee) allowing only the contract owner to enable or disable fee reimbursement. This emits an event to track changes off-chain:

  function setReimburseTxFee(bool _reimburseTxFee) external onlyOwner {
      reimburseTxFee = _reimburseTxFee;
      emit ReimburseTxFeeUpdated(_reimburseTxFee);
  }

• Modified finalizeDeposit Logic
  In finalizeDeposit(uint256 depositKey), we now check reimburseTxFee:

  if (reimburseTxFee) {
      tbtcAmount += txMaxFee;
  } 
  // The deduction happens on the AbstractTBTCDepositor contract (unchanged)

• Security Considerations

  • Only the contract owner can toggle reimburseTxFee.
  • If reimburseTxFee is true, the contract effectively reimburses the fee to the user.
  • If reimburseTxFee is false, the original fee deduction logic applies.

Key Goal:
This update keeps all existing integrations intact (default behavior remains unchanged) while giving new L2 deployments an option to fully refund the txMaxFee.

[lrsaturnino]

The Slither warning refers to:

Reentrancy in L1BitcoinDepositor.finalizeDeposit(uint256) (contracts/l2/L1BitcoinDepositor.sol#466-543):
	External calls:
	- (initialDepositAmount,tbtcAmount,l2DepositOwner) = _finalizeDeposit(depositKey) (contracts/l2/L1BitcoinDepositor.sol#476-482)
		- (finalizedAt) = tbtcVault.optimisticMintingRequests(depositKey) (contracts/integrator/AbstractTBTCDepositor.sol#[19](https://github.com/keep-network/tbtc-v2/actions/runs/13003732522/job/36315683377#step:9:20)1-193)
	Event emitted after the call(s):
	- DepositFinalized(depositKey,WormholeUtils.fromWormholeAddress(l2DepositOwner),msg.sender,initialDepositAmount,tbtcAmount) (contracts/l2/L1BitcoinDepositor.sol#494-500)

This happens because Slither sees an external call before an event emission and warns about potential reentrancy. However, that’s not an actual security breach, because the current logic prevents re-entry from doing anything harmful.

On finalizeDeposit function, it's set deposits[depositKey] = DepositState.Finalized before the external call. If an attacker tries to re-enter the function, it fails because the deposit is no longer in the Initialized state:

        require(
            deposits[depositKey] == DepositState.Initialized,
            "Wrong deposit state"
        );

Also, there's no function letting someone revert a Finalized deposit back to Initialized. Once final, it cannot be re-used maliciously.

To by-pass the Slither, I've put back the inline slither disabler:

        // slither-disable-next-line reentrancy-events
        emit DepositFinalized(
            depositKey,
            WormholeUtils.fromWormholeAddress(l2DepositOwner),
            msg.sender,
            initialDepositAmount,
            tbtcAmount
        );