From 69f52c21db30de70703c1fcfd682953c27dd07aa Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Thu, 8 Feb 2024 13:12:52 +0100 Subject: [PATCH] Don't track pending deposits in AbstractTBTCDepositor contract To optimize gas usage we removed `pendingDeposits` mapping. This operation reduces gas usage of `_initializeDeposit` function by ~20k gas. After this change it is implementation's contract responsibility to ensure `_finalizeDeposit` function cannot be called twice for the same deposit. `_initializeDeposit` function is not affected as the bridge blocks possibility of revealing the same deposit twice. --- .../integrator/AbstractTBTCDepositor.sol | 27 +++++++------------ .../integrator/AbstractTBTCDepositor.test.ts | 22 ++------------- 2 files changed, 12 insertions(+), 37 deletions(-) diff --git a/solidity/contracts/integrator/AbstractTBTCDepositor.sol b/solidity/contracts/integrator/AbstractTBTCDepositor.sol index 6f32b7130..e43f5d693 100644 --- a/solidity/contracts/integrator/AbstractTBTCDepositor.sol +++ b/solidity/contracts/integrator/AbstractTBTCDepositor.sol @@ -64,6 +64,9 @@ import "./ITBTCVault.sol"; /// } /// /// function finalizeProcess(uint256 depositKey) external { +/// // Ensure the function cannot be called for the same deposit +/// // twice. +/// /// ( /// uint256 initialDepositAmount, /// uint256 tbtcAmount, @@ -84,11 +87,6 @@ abstract contract AbstractTBTCDepositor { IBridge public bridge; /// @notice TBTCVault contract address. ITBTCVault public tbtcVault; - /// @notice Mapping holding information about pending deposits that have - /// been initialized but not finalized yet. If the deposit is not - /// in this mapping it means it has already been finalized or it - /// has not been initialized yet. - mapping(uint256 => bool) public pendingDeposits; // Reserved storage space that allows adding more variables without affecting // the storage layout of the child contracts. The convention from OpenZeppelin @@ -137,6 +135,8 @@ abstract contract AbstractTBTCDepositor { /// - The revealed vault address must match the TBTCVault address, /// - All requirements from {Bridge#revealDepositWithExtraData} /// function must be met. + /// @dev This function doesn't validate if a deposit has been initialized before, + /// as the Bridge won't allow the same deposit to be revealed twice. // slither-disable-next-line dead-code function _initializeDeposit( IBridgeTypes.BitcoinTxInfo calldata fundingTx, @@ -150,8 +150,6 @@ abstract contract AbstractTBTCDepositor { reveal.fundingOutputIndex ); - pendingDeposits[depositKey] = true; - emit DepositInitialized( depositKey, /* solhint-disable-next-line not-rely-on-time */ @@ -180,6 +178,9 @@ abstract contract AbstractTBTCDepositor { /// (in the context of this contract) yet. /// - The deposit must be finalized on the Bridge side. That means the /// deposit must be either swept or optimistically minted. + /// @dev This function doesn't validate if a deposit has been finalized before, + /// it is a responsibility of the implementing contract to ensure this + /// function won't be called twice for the same deposit. /// @dev IMPORTANT NOTE: The tbtcAmount returned by this function is an /// approximation. See documentation of the `calculateTbtcAmount` /// responsible for calculating this value for more details. @@ -192,11 +193,11 @@ abstract contract AbstractTBTCDepositor { bytes32 extraData ) { - require(pendingDeposits[depositKey], "Deposit not initialized"); - IBridgeTypes.DepositRequest memory deposit = bridge.deposits( depositKey ); + require(deposit.revealedAt != 0, "Deposit not initialized"); + (, uint64 finalizedAt) = tbtcVault.optimisticMintingRequests( depositKey ); @@ -206,14 +207,6 @@ abstract contract AbstractTBTCDepositor { "Deposit not finalized by the bridge" ); - // We can safely delete the deposit from the pending deposits mapping. - // This deposit cannot be initialized again because the bridge does not - // allow to reveal the same deposit twice. Deleting the deposit from - // the mapping will also prevent the finalizeDeposit function from - // being called again for the same deposit. - // slither-disable-next-line reentrancy-no-eth - delete pendingDeposits[depositKey]; - initialDepositAmount = deposit.amount * SATOSHI_MULTIPLIER; tbtcAmount = _calculateTbtcAmount(deposit.amount, deposit.treasuryFee); diff --git a/solidity/test/integrator/AbstractTBTCDepositor.test.ts b/solidity/test/integrator/AbstractTBTCDepositor.test.ts index 72a144462..7f77dfe15 100644 --- a/solidity/test/integrator/AbstractTBTCDepositor.test.ts +++ b/solidity/test/integrator/AbstractTBTCDepositor.test.ts @@ -131,12 +131,6 @@ describe("AbstractTBTCDepositor", () => { .withArgs(fixture.expectedDepositKey) }) - it("should store the deposit as pending", async () => { - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - expect(await depositor.pendingDeposits(fixture.expectedDepositKey)).to - .be.true - }) - it("should emit the DepositInitialized event", async () => { await expect(tx) .to.emit(depositor, "DepositInitialized") @@ -180,10 +174,10 @@ describe("AbstractTBTCDepositor", () => { await restoreSnapshot() }) - it("should revert", async () => { + it("should not revert", async () => { await expect( depositor.finalizeDepositPublic(fixture.expectedDepositKey) - ).to.be.revertedWith("Deposit not initialized") + ).to.be.not.reverted }) }) @@ -239,12 +233,6 @@ describe("AbstractTBTCDepositor", () => { await restoreSnapshot() }) - it("should remove the deposit from pending", async () => { - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - expect(await depositor.pendingDeposits(fixture.expectedDepositKey)) - .to.be.false - }) - it("should emit the DepositFinalized event", async () => { await expect(tx) .to.emit(depositor, "DepositFinalized") @@ -289,12 +277,6 @@ describe("AbstractTBTCDepositor", () => { await restoreSnapshot() }) - it("should remove the deposit from pending", async () => { - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - expect(await depositor.pendingDeposits(fixture.expectedDepositKey)) - .to.be.false - }) - it("should emit the DepositFinalized event", async () => { await expect(tx) .to.emit(depositor, "DepositFinalized")