From 1b95419737ea5d012647e0558e61a11b08feec86 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Aug 2024 12:20:07 +0200 Subject: [PATCH 01/10] Check if withdrawal amount exceeds the deposit balance Here we improve the withdrawal function to check if the withdrawal amount exceeds the deposit balance. If the withdrawal amount exceeds the deposit balance, the function should revert with an error message. Before these changes, the withdrawal function reverted on `depositBalance -= uint96(amount);` with the following error message: ``` panic code 0x11 (Arithmetic operation overflowed outside of an unchecked block) ``` --- solidity/contracts/MezoAllocator.sol | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/solidity/contracts/MezoAllocator.sol b/solidity/contracts/MezoAllocator.sol index d666c18c4..db5aae1ad 100644 --- a/solidity/contracts/MezoAllocator.sol +++ b/solidity/contracts/MezoAllocator.sol @@ -142,6 +142,12 @@ contract MezoAllocator is IDispatcher, Ownable2StepUpgradeable { error MaintainerNotRegistered(); /// @notice Reverts if the maintainer has been already registered. error MaintainerAlreadyRegistered(); + /// @notice Reverts if the requested amount to withdraw exceeds the amount + /// deposited in the Mezo Portal. + error WithdrawalAmountExceedsDepositBalance( + uint256 requestedAmount, + uint256 depositAmount + ); modifier onlyMaintainer() { if (!isMaintainer[msg.sender]) { @@ -233,6 +239,11 @@ contract MezoAllocator is IDispatcher, Ownable2StepUpgradeable { depositId, uint96(amount) ); + } else if (amount > depositBalance) { + revert WithdrawalAmountExceedsDepositBalance( + amount, + depositBalance + ); } else { mezoPortal.withdraw(address(tbtc), depositId); } From de1869ed906aee93ce2ed8703edb4373573d5eba Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Aug 2024 12:23:31 +0200 Subject: [PATCH 02/10] Add unit tests to cover WithdrawalAmountExceedsDepositBalance revert --- solidity/test/MezoAllocator.test.ts | 62 +++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 16 deletions(-) diff --git a/solidity/test/MezoAllocator.test.ts b/solidity/test/MezoAllocator.test.ts index 320dbb5ad..e0597cd14 100644 --- a/solidity/test/MezoAllocator.test.ts +++ b/solidity/test/MezoAllocator.test.ts @@ -264,78 +264,93 @@ describe("MezoAllocator", () => { context("when the caller is stBTC contract", () => { context("when there is no deposit", () => { it("should revert", async () => { - await expect( - stbtc.withdraw(1n, depositor, depositor), - ).to.be.revertedWithCustomError(mezoPortal, "DepositNotFound") + await expect(stbtc.withdraw(1n, depositor, depositor)) + .to.be.revertedWithCustomError( + mezoAllocator, + "WithdrawalAmountExceedsDepositBalance", + ) + .withArgs(1n, 0n) }) }) context("when there is a deposit", () => { + beforeAfterSnapshotWrapper() + + const depositAmount = to1e18(5) + let tx: ContractTransactionResponse before(async () => { - await tbtc.mint(depositor, to1e18(5)) - await tbtc.approve(await stbtc.getAddress(), to1e18(5)) - await stbtc.connect(depositor).deposit(to1e18(5), depositor) + await tbtc.mint(depositor, depositAmount) + await tbtc.approve(await stbtc.getAddress(), depositAmount) + await stbtc.connect(depositor).deposit(depositAmount, depositor) await mezoAllocator.connect(maintainer).allocate() }) context("when the deposit is not fully withdrawn", () => { + beforeAfterSnapshotWrapper() + + const withdrawalAmount = to1e18(2) + before(async () => { - tx = await stbtc.withdraw(to1e18(2), depositor, depositor) + tx = await stbtc.withdraw(withdrawalAmount, depositor, depositor) }) it("should transfer 2 tBTC back to a depositor", async () => { await expect(tx).to.changeTokenBalances( tbtc, [depositor.address], - [to1e18(2)], + [withdrawalAmount], ) }) it("should emit DepositWithdrawn event", async () => { await expect(tx) .to.emit(mezoAllocator, "DepositWithdrawn") - .withArgs(1, to1e18(2)) + .withArgs(1, withdrawalAmount) }) it("should decrease tracked deposit balance amount", async () => { const depositBalance = await mezoAllocator.depositBalance() - expect(depositBalance).to.equal(to1e18(3)) + expect(depositBalance).to.equal(depositAmount - withdrawalAmount) }) it("should decrease Mezo Portal balance", async () => { await expect(tx).to.changeTokenBalances( tbtc, [await mezoPortal.getAddress()], - [-to1e18(2)], + [-withdrawalAmount], ) }) it("should call MezoPortal.withdrawPartially function", async () => { await expect(tx) .to.emit(mezoPortal, "WithdrawPartially") - .withArgs(await tbtc.getAddress(), 1, to1e18(2)) + .withArgs(await tbtc.getAddress(), 1, withdrawalAmount) }) }) context("when the deposit is fully withdrawn", () => { + beforeAfterSnapshotWrapper() + + const withdrawalAmount = depositAmount + before(async () => { - tx = await stbtc.withdraw(to1e18(3), depositor, depositor) + tx = await stbtc.withdraw(withdrawalAmount, depositor, depositor) }) it("should transfer 3 tBTC back to a depositor", async () => { await expect(tx).to.changeTokenBalances( tbtc, [depositor.address], - [to1e18(3)], + [withdrawalAmount], ) }) it("should emit DepositWithdrawn event", async () => { await expect(tx) .to.emit(mezoAllocator, "DepositWithdrawn") - .withArgs(1, to1e18(3)) + .withArgs(1, withdrawalAmount) }) it("should decrease tracked deposit balance amount to zero", async () => { @@ -347,7 +362,7 @@ describe("MezoAllocator", () => { await expect(tx).to.changeTokenBalances( tbtc, [await mezoPortal.getAddress()], - [-to1e18(3)], + [-withdrawalAmount], ) }) @@ -357,6 +372,21 @@ describe("MezoAllocator", () => { .withArgs(await tbtc.getAddress(), 1) }) }) + + context("when requested amount exceeds the deposit balance", () => { + beforeAfterSnapshotWrapper() + + const withdrawalAmount = depositAmount + 1n + + it("should revert", async () => { + await expect(stbtc.withdraw(withdrawalAmount, depositor, depositor)) + .to.be.revertedWithCustomError( + mezoAllocator, + "WithdrawalAmountExceedsDepositBalance", + ) + .withArgs(withdrawalAmount, depositAmount) + }) + }) }) }) }) From 1b9ba17fe3df251445dc36b6b6452c3dd242ccb0 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Aug 2024 13:14:19 +0200 Subject: [PATCH 03/10] Include unallocated MezoAllocator balance in withdraw In MezoAllocator contract in the totalAssets function we include the balance of the deposit made in Mezo Portal as well as the balance of the MezoAllocator contract that hasn't been yet allocated (this can be due to a donation or rewards). This change is to include the unallocated balance in the withdraw function so that the user can withdraw the unallocated balance as well. If we're not withdrawing the unallocated balance, a last user to withdraw may not be able to complete the withdrawal as the amount of tBTC that is transferred to the stBTC contract includes only the funds withdrawn from the Mezo Portal contract. --- solidity/contracts/MezoAllocator.sol | 41 ++++++++++++++++------------ 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/solidity/contracts/MezoAllocator.sol b/solidity/contracts/MezoAllocator.sol index db5aae1ad..f456f76f9 100644 --- a/solidity/contracts/MezoAllocator.sol +++ b/solidity/contracts/MezoAllocator.sol @@ -231,25 +231,32 @@ contract MezoAllocator is IDispatcher, Ownable2StepUpgradeable { function withdraw(uint256 amount) external { if (msg.sender != address(stbtc)) revert CallerNotStbtc(); - emit DepositWithdrawn(depositId, amount); - - if (amount < depositBalance) { - mezoPortal.withdrawPartially( - address(tbtc), - depositId, - uint96(amount) - ); - } else if (amount > depositBalance) { - revert WithdrawalAmountExceedsDepositBalance( - amount, - depositBalance - ); - } else { - mezoPortal.withdraw(address(tbtc), depositId); + uint256 unallocatedBalance = tbtc.balanceOf(address(this)); + + if (amount > unallocatedBalance) { + uint256 amountToWithdraw = amount - unallocatedBalance; + + emit DepositWithdrawn(depositId, amountToWithdraw); + + if (amountToWithdraw < depositBalance) { + mezoPortal.withdrawPartially( + address(tbtc), + depositId, + uint96(amountToWithdraw) + ); + } else if (amountToWithdraw > depositBalance) { + revert WithdrawalAmountExceedsDepositBalance( + amountToWithdraw, + depositBalance + ); + } else { + mezoPortal.withdraw(address(tbtc), depositId); + } + + // slither-disable-next-line reentrancy-no-eth + depositBalance -= uint96(amountToWithdraw); } - // slither-disable-next-line reentrancy-no-eth - depositBalance -= uint96(amount); tbtc.safeTransfer(address(stbtc), amount); } From be2c530576022ff1ef8010c8ef4536c37ea87b24 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Aug 2024 14:57:42 +0200 Subject: [PATCH 04/10] Add unit tests for MezoAllocator.withdraw function Cover the latest changes of withdrawing the unallocated balance with unit tests. We fake stBTC contract to simulate the withdraw function call, as msg.sender has to be the stBTC contract. --- solidity/test/MezoAllocator.test.ts | 347 ++++++++++++++++++++-------- 1 file changed, 254 insertions(+), 93 deletions(-) diff --git a/solidity/test/MezoAllocator.test.ts b/solidity/test/MezoAllocator.test.ts index e0597cd14..b8960e6ae 100644 --- a/solidity/test/MezoAllocator.test.ts +++ b/solidity/test/MezoAllocator.test.ts @@ -1,7 +1,12 @@ -import { helpers } from "hardhat" +import { helpers, ethers } from "hardhat" import { HardhatEthersSigner } from "@nomicfoundation/hardhat-ethers/signers" import { expect } from "chai" -import { loadFixture } from "@nomicfoundation/hardhat-toolbox/network-helpers" +import { + impersonateAccount, + loadFixture, + setBalance, + stopImpersonatingAccount, +} from "@nomicfoundation/hardhat-toolbox/network-helpers" import { ContractTransactionResponse, ZeroAddress } from "ethers" import { beforeAfterSnapshotWrapper, deployment } from "./helpers" @@ -46,6 +51,7 @@ describe("MezoAllocator", () => { let depositor2: HardhatEthersSigner let maintainer: HardhatEthersSigner let governance: HardhatEthersSigner + let stbtcFakeSigner: HardhatEthersSigner before(async () => { ;({ @@ -62,6 +68,15 @@ describe("MezoAllocator", () => { await stbtc.connect(governance).updateEntryFeeBasisPoints(0) await stbtc.connect(governance).updateExitFeeBasisPoints(0) + + // Impersonate stBTC contract to be able to fake msg.sender. + await impersonateAccount(await stbtc.getAddress()) + stbtcFakeSigner = await ethers.getSigner(await stbtc.getAddress()) + await setBalance(stbtcFakeSigner.address, to1e18(1)) + }) + + after(async () => { + await stopImpersonatingAccount(await stbtc.getAddress()) }) describe("allocate", () => { @@ -264,7 +279,7 @@ describe("MezoAllocator", () => { context("when the caller is stBTC contract", () => { context("when there is no deposit", () => { it("should revert", async () => { - await expect(stbtc.withdraw(1n, depositor, depositor)) + await expect(mezoAllocator.connect(stbtcFakeSigner).withdraw(1n)) .to.be.revertedWithCustomError( mezoAllocator, "WithdrawalAmountExceedsDepositBalance", @@ -274,6 +289,140 @@ describe("MezoAllocator", () => { }) context("when there is a deposit", () => { + const testWithdrawalFromMezoPortal = ({ + initialDepositBalance, + partialWithdrawal, + fullWithdrawal, + }: { + initialDepositBalance: bigint + partialWithdrawal: { + requestedAmount: bigint + expectedWithdrawnAmount: bigint + } + fullWithdrawal: { + requestedAmount: bigint + expectedWithdrawnAmount: bigint + expectMaxWithdrawalExceeded?: boolean + } + }) => + // eslint-disable-next-line func-names + function () { + beforeAfterSnapshotWrapper() + + context("when the deposit is partially withdrawn", () => { + beforeAfterSnapshotWrapper() + + before(async () => { + tx = await mezoAllocator + .connect(stbtcFakeSigner) + .withdraw(partialWithdrawal.requestedAmount) + }) + + it("should transfer tBTC to stBTC contract", async () => { + await expect(tx).to.changeTokenBalances( + tbtc, + [await stbtc.getAddress()], + [partialWithdrawal.requestedAmount], + ) + }) + + it("should emit DepositWithdrawn event", async () => { + await expect(tx) + .to.emit(mezoAllocator, "DepositWithdrawn") + .withArgs(1, partialWithdrawal.expectedWithdrawnAmount) + }) + + it("should decrease tracked deposit balance amount", async () => { + const depositBalance = await mezoAllocator.depositBalance() + expect(depositBalance).to.equal( + initialDepositBalance - + partialWithdrawal.expectedWithdrawnAmount, + ) + }) + + it("should decrease Mezo Portal balance", async () => { + await expect(tx).to.changeTokenBalances( + tbtc, + [await mezoPortal.getAddress()], + [-partialWithdrawal.expectedWithdrawnAmount], + ) + }) + + it("should call MezoPortal.withdrawPartially function", async () => { + await expect(tx) + .to.emit(mezoPortal, "WithdrawPartially") + .withArgs( + await tbtc.getAddress(), + 1, + partialWithdrawal.expectedWithdrawnAmount, + ) + }) + }) + + context("when the deposit is fully withdrawn", () => { + beforeAfterSnapshotWrapper() + + before(async () => { + tx = await mezoAllocator + .connect(stbtcFakeSigner) + .withdraw(fullWithdrawal.requestedAmount) + }) + + it("should transfer tBTC to stBTC contract", async () => { + await expect(tx).to.changeTokenBalances( + tbtc, + [await stbtc.getAddress()], + [fullWithdrawal.requestedAmount], + ) + }) + + it("should emit DepositWithdrawn event", async () => { + await expect(tx) + .to.emit(mezoAllocator, "DepositWithdrawn") + .withArgs(1, fullWithdrawal.expectedWithdrawnAmount) + }) + + it("should decrease tracked deposit balance amount to zero", async () => { + const depositBalance = await mezoAllocator.depositBalance() + expect(depositBalance).to.equal(0) + }) + + it("should decrease Mezo Portal balance", async () => { + await expect(tx).to.changeTokenBalances( + tbtc, + [await mezoPortal.getAddress()], + [-fullWithdrawal.expectedWithdrawnAmount], + ) + }) + + it("should call MezoPortal.withdraw function", async () => { + await expect(tx) + .to.emit(mezoPortal, "WithdrawFully") + .withArgs(await tbtc.getAddress(), 1) + }) + }) + + context("when requested amount exceeds the deposit balance", () => { + beforeAfterSnapshotWrapper() + + it("should revert", async () => { + await expect( + mezoAllocator + .connect(stbtcFakeSigner) + .withdraw(fullWithdrawal.requestedAmount + 1n), + ) + .to.be.revertedWithCustomError( + mezoAllocator, + "WithdrawalAmountExceedsDepositBalance", + ) + .withArgs( + fullWithdrawal.expectedWithdrawnAmount + 1n, + initialDepositBalance, + ) + }) + }) + } + beforeAfterSnapshotWrapper() const depositAmount = to1e18(5) @@ -287,105 +436,117 @@ describe("MezoAllocator", () => { await mezoAllocator.connect(maintainer).allocate() }) - context("when the deposit is not fully withdrawn", () => { - beforeAfterSnapshotWrapper() - - const withdrawalAmount = to1e18(2) - - before(async () => { - tx = await stbtc.withdraw(withdrawalAmount, depositor, depositor) - }) - - it("should transfer 2 tBTC back to a depositor", async () => { - await expect(tx).to.changeTokenBalances( - tbtc, - [depositor.address], - [withdrawalAmount], - ) - }) - - it("should emit DepositWithdrawn event", async () => { - await expect(tx) - .to.emit(mezoAllocator, "DepositWithdrawn") - .withArgs(1, withdrawalAmount) - }) - - it("should decrease tracked deposit balance amount", async () => { - const depositBalance = await mezoAllocator.depositBalance() - expect(depositBalance).to.equal(depositAmount - withdrawalAmount) - }) - - it("should decrease Mezo Portal balance", async () => { - await expect(tx).to.changeTokenBalances( - tbtc, - [await mezoPortal.getAddress()], - [-withdrawalAmount], - ) - }) - - it("should call MezoPortal.withdrawPartially function", async () => { - await expect(tx) - .to.emit(mezoPortal, "WithdrawPartially") - .withArgs(await tbtc.getAddress(), 1, withdrawalAmount) - }) - }) + context( + "when there is no donation made", + testWithdrawalFromMezoPortal({ + initialDepositBalance: depositAmount, + partialWithdrawal: { + requestedAmount: depositAmount - 1n, + expectedWithdrawnAmount: depositAmount - 1n, + }, + fullWithdrawal: { + requestedAmount: depositAmount, + expectedWithdrawnAmount: depositAmount, + }, + }), + ) - context("when the deposit is fully withdrawn", () => { + context("when there is a donation made", () => { beforeAfterSnapshotWrapper() - const withdrawalAmount = depositAmount + const donationAmount = to1e18(1n) before(async () => { - tx = await stbtc.withdraw(withdrawalAmount, depositor, depositor) - }) - - it("should transfer 3 tBTC back to a depositor", async () => { - await expect(tx).to.changeTokenBalances( - tbtc, - [depositor.address], - [withdrawalAmount], - ) + await tbtc.mint(await mezoAllocator.getAddress(), donationAmount) }) - it("should emit DepositWithdrawn event", async () => { - await expect(tx) - .to.emit(mezoAllocator, "DepositWithdrawn") - .withArgs(1, withdrawalAmount) - }) - - it("should decrease tracked deposit balance amount to zero", async () => { - const depositBalance = await mezoAllocator.depositBalance() - expect(depositBalance).to.equal(0) - }) - - it("should decrease Mezo Portal balance", async () => { - await expect(tx).to.changeTokenBalances( - tbtc, - [await mezoPortal.getAddress()], - [-withdrawalAmount], - ) - }) - - it("should call MezoPortal.withdraw function", async () => { - await expect(tx) - .to.emit(mezoPortal, "WithdrawFully") - .withArgs(await tbtc.getAddress(), 1) - }) - }) - - context("when requested amount exceeds the deposit balance", () => { - beforeAfterSnapshotWrapper() + context( + "when requested amount is lower than unallocated balance", + () => { + beforeAfterSnapshotWrapper() + + const withdrawalAmount = donationAmount - 1n + + before(async () => { + tx = await mezoAllocator + .connect(stbtcFakeSigner) + .withdraw(withdrawalAmount) + }) + + it("should transfer tBTC to stBTC contract", async () => { + await expect(tx).to.changeTokenBalances( + tbtc, + [await stbtc.getAddress()], + [withdrawalAmount], + ) + }) + + it("should NOT emit DepositWithdrawn event", async () => { + await expect(tx).to.not.emit(mezoAllocator, "DepositWithdrawn") + }) + + it("should NOT decrease tracked deposit balance amount", async () => { + const depositBalance = await mezoAllocator.depositBalance() + expect(depositBalance).to.equal(depositAmount) + }) + + it("should NOT call MezoPortal.withdrawPartially function", async () => { + await expect(tx).to.not.emit(mezoPortal, "WithdrawPartially") + }) + }, + ) - const withdrawalAmount = depositAmount + 1n + context( + "when requested amount is equal to unallocated balance", + () => { + beforeAfterSnapshotWrapper() + + const withdrawalAmount = donationAmount + + before(async () => { + tx = await mezoAllocator + .connect(stbtcFakeSigner) + .withdraw(withdrawalAmount) + }) + + it("should transfer tBTC to stBTC contract", async () => { + await expect(tx).to.changeTokenBalances( + tbtc, + [await stbtc.getAddress()], + [withdrawalAmount], + ) + }) + + it("should NOT emit DepositWithdrawn event", async () => { + await expect(tx).to.not.emit(mezoAllocator, "DepositWithdrawn") + }) + + it("should NOT decrease tracked deposit balance amount", async () => { + const depositBalance = await mezoAllocator.depositBalance() + expect(depositBalance).to.equal(depositAmount) + }) + + it("should NOT call MezoPortal.withdrawPartially function", async () => { + await expect(tx).to.not.emit(mezoPortal, "WithdrawPartially") + }) + }, + ) - it("should revert", async () => { - await expect(stbtc.withdraw(withdrawalAmount, depositor, depositor)) - .to.be.revertedWithCustomError( - mezoAllocator, - "WithdrawalAmountExceedsDepositBalance", - ) - .withArgs(withdrawalAmount, depositAmount) - }) + context( + "when requested amount exceeds the unallocated balance", + testWithdrawalFromMezoPortal({ + initialDepositBalance: depositAmount, + partialWithdrawal: { + requestedAmount: donationAmount + 1n, + expectedWithdrawnAmount: 1n, + }, + fullWithdrawal: { + requestedAmount: donationAmount + depositAmount, + expectedWithdrawnAmount: depositAmount, + expectMaxWithdrawalExceeded: true, + }, + }), + ) }) }) }) From 0440580dcfcc879eb417d07033c30ed33af3e3a2 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 28 Aug 2024 15:12:43 +0200 Subject: [PATCH 05/10] Include unallocated balance in releaseDeposit function We were not including the unallocated balance in the releaseDeposit function. If there was no Mezo deposit, but some funds were donated to the contract they would be stuck there forever. This change allows the contract to release both unallocated balance and Mezo deposit. --- solidity/contracts/MezoAllocator.sol | 9 +- solidity/test/MezoAllocator.test.ts | 144 +++++++++++++++++++++++---- 2 files changed, 133 insertions(+), 20 deletions(-) diff --git a/solidity/contracts/MezoAllocator.sol b/solidity/contracts/MezoAllocator.sol index f456f76f9..43bfbf33f 100644 --- a/solidity/contracts/MezoAllocator.sol +++ b/solidity/contracts/MezoAllocator.sol @@ -268,9 +268,12 @@ contract MezoAllocator is IDispatcher, Ownable2StepUpgradeable { .getDeposit(address(this), address(tbtc), depositId) .balance; - emit DepositReleased(depositId, amount); - depositBalance = 0; - mezoPortal.withdraw(address(tbtc), depositId); + if (amount > 0) { + emit DepositReleased(depositId, amount); + depositBalance = 0; + mezoPortal.withdraw(address(tbtc), depositId); + } + tbtc.safeTransfer(address(stbtc), tbtc.balanceOf(address(this))); } diff --git a/solidity/test/MezoAllocator.test.ts b/solidity/test/MezoAllocator.test.ts index b8960e6ae..946aac5aa 100644 --- a/solidity/test/MezoAllocator.test.ts +++ b/solidity/test/MezoAllocator.test.ts @@ -733,32 +733,142 @@ describe("MezoAllocator", () => { }) context("when the caller is governance", () => { + beforeAfterSnapshotWrapper() + + context("when there is no deposit", () => { + beforeAfterSnapshotWrapper() + + context("when there is no donation", () => { + beforeAfterSnapshotWrapper() + + let tx: ContractTransactionResponse + + before(async () => { + tx = await mezoAllocator.connect(governance).releaseDeposit() + }) + + it("should not emit DepositReleased event", async () => { + await expect(tx).to.not.emit(mezoAllocator, "DepositReleased") + }) + + it("should not call MezoPortal.withdraw function", async () => { + await expect(tx).to.not.emit(mezoPortal, "WithdrawFully") + }) + }) + + context("when there is a donation", () => { + beforeAfterSnapshotWrapper() + + const donationAmount = to1e18(2) + + let tx: ContractTransactionResponse + + before(async () => { + await tbtc.mint(await mezoAllocator.getAddress(), donationAmount) + + tx = await mezoAllocator.connect(governance).releaseDeposit() + }) + + it("should not emit DepositReleased event", async () => { + await expect(tx).to.not.emit(mezoAllocator, "DepositReleased") + }) + + it("should transfer tBTC to stBTC contract", async () => { + await expect(tx).to.changeTokenBalances( + tbtc, + [mezoAllocator, stbtc], + [-donationAmount, donationAmount], + ) + }) + + it("should not call MezoPortal.withdraw function", async () => { + await expect(tx).to.not.emit(mezoPortal, "WithdrawFully") + }) + }) + }) + context("when there is a deposit", () => { - let tx: ContractTransactionResponse + beforeAfterSnapshotWrapper() + + const depositAmount = to1e18(5) before(async () => { - await tbtc.mint(await stbtc.getAddress(), to1e18(5)) + await tbtc.mint(await stbtc.getAddress(), depositAmount) await mezoAllocator.connect(maintainer).allocate() - tx = await mezoAllocator.connect(governance).releaseDeposit() }) - it("should emit DepositReleased event", async () => { - await expect(tx) - .to.emit(mezoAllocator, "DepositReleased") - .withArgs(1, to1e18(5)) - }) + context("when there is no donation", () => { + beforeAfterSnapshotWrapper() + + let tx: ContractTransactionResponse + + before(async () => { + tx = await mezoAllocator.connect(governance).releaseDeposit() + }) + + it("should emit DepositReleased event", async () => { + await expect(tx) + .to.emit(mezoAllocator, "DepositReleased") + .withArgs(1, depositAmount) + }) + + it("should decrease tracked deposit balance amount to zero", async () => { + const depositBalance = await mezoAllocator.depositBalance() + expect(depositBalance).to.equal(0) + }) + + it("should transfer tBTC to stBTC contract", async () => { + await expect(tx).to.changeTokenBalances( + tbtc, + [mezoPortal, stbtc], + [-depositAmount, depositAmount], + ) + }) - it("should decrease tracked deposit balance amount to zero", async () => { - const depositBalance = await mezoAllocator.depositBalance() - expect(depositBalance).to.equal(0) + it("should call MezoPortal.withdraw function", async () => { + await expect(tx) + .to.emit(mezoPortal, "WithdrawFully") + .withArgs(await tbtc.getAddress(), 1) + }) }) - it("should decrease Mezo Portal balance", async () => { - await expect(tx).to.changeTokenBalances( - tbtc, - [mezoPortal, stbtc], - [-to1e18(5), to1e18(5)], - ) + context("when there is a donation", () => { + beforeAfterSnapshotWrapper() + + const donationAmount = to1e18(2) + + let tx: ContractTransactionResponse + + before(async () => { + await tbtc.mint(await mezoAllocator.getAddress(), donationAmount) + + tx = await mezoAllocator.connect(governance).releaseDeposit() + }) + + it("should emit DepositReleased event", async () => { + await expect(tx) + .to.emit(mezoAllocator, "DepositReleased") + .withArgs(1, depositAmount) + }) + + it("should decrease tracked deposit balance amount to zero", async () => { + const depositBalance = await mezoAllocator.depositBalance() + expect(depositBalance).to.equal(0) + }) + + it("should transfer tBTC to stBTC contract", async () => { + await expect(tx).to.changeTokenBalances( + tbtc, + [mezoPortal, mezoAllocator, stbtc], + [-depositAmount, -donationAmount, depositAmount + donationAmount], + ) + }) + + it("should call MezoPortal.withdraw function", async () => { + await expect(tx) + .to.emit(mezoPortal, "WithdrawFully") + .withArgs(await tbtc.getAddress(), 1) + }) }) }) }) From 93a781f0aced7c7a5f2a7c7e0cb3be0a0bf47077 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Fri, 30 Aug 2024 13:30:03 +0200 Subject: [PATCH 06/10] Extract depositId variable for MezoAllocator tests It helps to read the tests when the depositId is extracted into a variable. --- solidity/test/MezoAllocator.test.ts | 42 ++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/solidity/test/MezoAllocator.test.ts b/solidity/test/MezoAllocator.test.ts index 946aac5aa..ae78179c2 100644 --- a/solidity/test/MezoAllocator.test.ts +++ b/solidity/test/MezoAllocator.test.ts @@ -94,6 +94,9 @@ describe("MezoAllocator", () => { context("when two consecutive deposits are made", () => { beforeAfterSnapshotWrapper() + const initialDepositId = 0 + const expectedDepositId = 1 + context("when a first deposit is made", () => { let tx: ContractTransactionResponse @@ -118,7 +121,7 @@ describe("MezoAllocator", () => { it("should increment the deposit id", async () => { const actualDepositId = await mezoAllocator.depositId() - expect(actualDepositId).to.equal(1) + expect(actualDepositId).to.equal(expectedDepositId) }) it("should increase tracked deposit balance amount", async () => { @@ -129,7 +132,12 @@ describe("MezoAllocator", () => { it("should emit DepositAllocated event", async () => { await expect(tx) .to.emit(mezoAllocator, "DepositAllocated") - .withArgs(0, 1, to1e18(6), to1e18(6)) + .withArgs( + initialDepositId, + expectedDepositId, + to1e18(6), + to1e18(6), + ) }) }) @@ -144,13 +152,18 @@ describe("MezoAllocator", () => { it("should increment the deposit id", async () => { const actualDepositId = await mezoAllocator.depositId() - expect(actualDepositId).to.equal(2) + expect(actualDepositId).to.equal(expectedDepositId + 1) }) it("should emit DepositAllocated event", async () => { await expect(tx) .to.emit(mezoAllocator, "DepositAllocated") - .withArgs(1, 2, to1e18(5), to1e18(11)) + .withArgs( + expectedDepositId, + expectedDepositId + 1, + to1e18(5), + to1e18(11), + ) }) it("should deposit and transfer tBTC to Mezo Portal", async () => { @@ -289,6 +302,8 @@ describe("MezoAllocator", () => { }) context("when there is a deposit", () => { + const depositId = 1 + const testWithdrawalFromMezoPortal = ({ initialDepositBalance, partialWithdrawal, @@ -329,7 +344,10 @@ describe("MezoAllocator", () => { it("should emit DepositWithdrawn event", async () => { await expect(tx) .to.emit(mezoAllocator, "DepositWithdrawn") - .withArgs(1, partialWithdrawal.expectedWithdrawnAmount) + .withArgs( + depositId, + partialWithdrawal.expectedWithdrawnAmount, + ) }) it("should decrease tracked deposit balance amount", async () => { @@ -353,7 +371,7 @@ describe("MezoAllocator", () => { .to.emit(mezoPortal, "WithdrawPartially") .withArgs( await tbtc.getAddress(), - 1, + depositId, partialWithdrawal.expectedWithdrawnAmount, ) }) @@ -379,7 +397,7 @@ describe("MezoAllocator", () => { it("should emit DepositWithdrawn event", async () => { await expect(tx) .to.emit(mezoAllocator, "DepositWithdrawn") - .withArgs(1, fullWithdrawal.expectedWithdrawnAmount) + .withArgs(depositId, fullWithdrawal.expectedWithdrawnAmount) }) it("should decrease tracked deposit balance amount to zero", async () => { @@ -398,7 +416,7 @@ describe("MezoAllocator", () => { it("should call MezoPortal.withdraw function", async () => { await expect(tx) .to.emit(mezoPortal, "WithdrawFully") - .withArgs(await tbtc.getAddress(), 1) + .withArgs(await tbtc.getAddress(), depositId) }) }) @@ -809,7 +827,7 @@ describe("MezoAllocator", () => { it("should emit DepositReleased event", async () => { await expect(tx) .to.emit(mezoAllocator, "DepositReleased") - .withArgs(1, depositAmount) + .withArgs(depositId, depositAmount) }) it("should decrease tracked deposit balance amount to zero", async () => { @@ -828,7 +846,7 @@ describe("MezoAllocator", () => { it("should call MezoPortal.withdraw function", async () => { await expect(tx) .to.emit(mezoPortal, "WithdrawFully") - .withArgs(await tbtc.getAddress(), 1) + .withArgs(await tbtc.getAddress(), depositId) }) }) @@ -848,7 +866,7 @@ describe("MezoAllocator", () => { it("should emit DepositReleased event", async () => { await expect(tx) .to.emit(mezoAllocator, "DepositReleased") - .withArgs(1, depositAmount) + .withArgs(depositId, depositAmount) }) it("should decrease tracked deposit balance amount to zero", async () => { @@ -867,7 +885,7 @@ describe("MezoAllocator", () => { it("should call MezoPortal.withdraw function", async () => { await expect(tx) .to.emit(mezoPortal, "WithdrawFully") - .withArgs(await tbtc.getAddress(), 1) + .withArgs(await tbtc.getAddress(), depositId) }) }) }) From 1198ea0e3c7093e83cf3622ffefcdcd88a6b654c Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Fri, 30 Aug 2024 14:00:53 +0200 Subject: [PATCH 07/10] Define missing depositId variable --- solidity/test/MezoAllocator.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/solidity/test/MezoAllocator.test.ts b/solidity/test/MezoAllocator.test.ts index ae78179c2..deb2297d6 100644 --- a/solidity/test/MezoAllocator.test.ts +++ b/solidity/test/MezoAllocator.test.ts @@ -808,6 +808,7 @@ describe("MezoAllocator", () => { context("when there is a deposit", () => { beforeAfterSnapshotWrapper() + const depositId = 1 const depositAmount = to1e18(5) before(async () => { From dcbe752a3593b3d4af352a61e76926bb0dd0d3e6 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Fri, 30 Aug 2024 14:10:10 +0200 Subject: [PATCH 08/10] Add event to track withdraws from MezoAllocator balance We were emitting event when funds were withdrawn from MezoPortal. But it's possible that withdrawal will be satisfied from MezoAllocator balance, so we added additional event. --- solidity/contracts/MezoAllocator.sol | 14 ++++++++-- solidity/test/MezoAllocator.test.ts | 41 ++++++++++++++++++++++------ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/solidity/contracts/MezoAllocator.sol b/solidity/contracts/MezoAllocator.sol index 43bfbf33f..81ecad193 100644 --- a/solidity/contracts/MezoAllocator.sol +++ b/solidity/contracts/MezoAllocator.sol @@ -127,7 +127,15 @@ contract MezoAllocator is IDispatcher, Ownable2StepUpgradeable { uint256 newDepositAmount ); /// @notice Emitted when tBTC is withdrawn from MezoPortal. - event DepositWithdrawn(uint256 indexed depositId, uint256 amount); + /// If MezoAllocator has a positive balance part of the requested amount + /// is withdrawn from MezoAllocator and the rest from MezoPortal. + event WithdrawFromMezoPortal( + uint256 indexed depositId, + uint256 requestedAmount, + uint256 amountWithdrawnFromPortal + ); + /// @notice Emitted when tBTC is withdrawn from MezoAllocator. + event WithdrawFromMezoAllocator(uint256 amount); /// @notice Emitted when the maintainer address is updated. event MaintainerAdded(address indexed maintainer); /// @notice Emitted when the maintainer address is updated. @@ -236,7 +244,7 @@ contract MezoAllocator is IDispatcher, Ownable2StepUpgradeable { if (amount > unallocatedBalance) { uint256 amountToWithdraw = amount - unallocatedBalance; - emit DepositWithdrawn(depositId, amountToWithdraw); + emit WithdrawFromMezoPortal(depositId, amount, amountToWithdraw); if (amountToWithdraw < depositBalance) { mezoPortal.withdrawPartially( @@ -255,6 +263,8 @@ contract MezoAllocator is IDispatcher, Ownable2StepUpgradeable { // slither-disable-next-line reentrancy-no-eth depositBalance -= uint96(amountToWithdraw); + } else { + emit WithdrawFromMezoAllocator(amount); } tbtc.safeTransfer(address(stbtc), amount); diff --git a/solidity/test/MezoAllocator.test.ts b/solidity/test/MezoAllocator.test.ts index deb2297d6..b6d872831 100644 --- a/solidity/test/MezoAllocator.test.ts +++ b/solidity/test/MezoAllocator.test.ts @@ -341,11 +341,12 @@ describe("MezoAllocator", () => { ) }) - it("should emit DepositWithdrawn event", async () => { + it("should emit WithdrawFromMezoPortal event", async () => { await expect(tx) - .to.emit(mezoAllocator, "DepositWithdrawn") + .to.emit(mezoAllocator, "WithdrawFromMezoPortal") .withArgs( depositId, + partialWithdrawal.requestedAmount, partialWithdrawal.expectedWithdrawnAmount, ) }) @@ -394,10 +395,14 @@ describe("MezoAllocator", () => { ) }) - it("should emit DepositWithdrawn event", async () => { + it("should emit WithdrawFromMezoPortal event", async () => { await expect(tx) - .to.emit(mezoAllocator, "DepositWithdrawn") - .withArgs(depositId, fullWithdrawal.expectedWithdrawnAmount) + .to.emit(mezoAllocator, "WithdrawFromMezoPortal") + .withArgs( + depositId, + fullWithdrawal.requestedAmount, + fullWithdrawal.expectedWithdrawnAmount, + ) }) it("should decrease tracked deposit balance amount to zero", async () => { @@ -499,8 +504,17 @@ describe("MezoAllocator", () => { ) }) - it("should NOT emit DepositWithdrawn event", async () => { - await expect(tx).to.not.emit(mezoAllocator, "DepositWithdrawn") + it("should emit WithdrawFromMezoAllocator event", async () => { + await expect(tx) + .to.emit(mezoAllocator, "WithdrawFromMezoAllocator") + .withArgs(withdrawalAmount) + }) + + it("should NOT emit WithdrawFromMezoPortal event", async () => { + await expect(tx).to.not.emit( + mezoAllocator, + "WithdrawFromMezoPortal", + ) }) it("should NOT decrease tracked deposit balance amount", async () => { @@ -535,8 +549,17 @@ describe("MezoAllocator", () => { ) }) - it("should NOT emit DepositWithdrawn event", async () => { - await expect(tx).to.not.emit(mezoAllocator, "DepositWithdrawn") + it("should emit WithdrawFromMezoAllocator event", async () => { + await expect(tx) + .to.emit(mezoAllocator, "WithdrawFromMezoAllocator") + .withArgs(withdrawalAmount) + }) + + it("should NOT emit WithdrawFromMezoPortal event", async () => { + await expect(tx).to.not.emit( + mezoAllocator, + "WithdrawFromMezoPortal", + ) }) it("should NOT decrease tracked deposit balance amount", async () => { From 7b97aac7f45a59b73a9e73f505e3d9e1aa1f922a Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Fri, 30 Aug 2024 14:21:13 +0200 Subject: [PATCH 09/10] Update withdraw statements sequence Cleaned up the sequence to move the error to the last statement. --- solidity/contracts/MezoAllocator.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/solidity/contracts/MezoAllocator.sol b/solidity/contracts/MezoAllocator.sol index 81ecad193..0929d4fdc 100644 --- a/solidity/contracts/MezoAllocator.sol +++ b/solidity/contracts/MezoAllocator.sol @@ -252,13 +252,13 @@ contract MezoAllocator is IDispatcher, Ownable2StepUpgradeable { depositId, uint96(amountToWithdraw) ); - } else if (amountToWithdraw > depositBalance) { + } else if (amountToWithdraw == depositBalance) { + mezoPortal.withdraw(address(tbtc), depositId); + } else { revert WithdrawalAmountExceedsDepositBalance( amountToWithdraw, depositBalance ); - } else { - mezoPortal.withdraw(address(tbtc), depositId); } // slither-disable-next-line reentrancy-no-eth From 6151a386493b559d264bf20c45ceb90d72c83e43 Mon Sep 17 00:00:00 2001 From: Jakub Date: Tue, 3 Sep 2024 11:16:30 +0200 Subject: [PATCH 10/10] Ignore Slither's incorrect equality rule in MezoAllocator::withdraw We want to execute the Portal::withdraw function only if it's a total withdrawal. If there is a donation to the MezoAllocator, we will use the partial withdrawal function. --- solidity/contracts/MezoAllocator.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/solidity/contracts/MezoAllocator.sol b/solidity/contracts/MezoAllocator.sol index 0929d4fdc..07682f9b3 100644 --- a/solidity/contracts/MezoAllocator.sol +++ b/solidity/contracts/MezoAllocator.sol @@ -252,6 +252,7 @@ contract MezoAllocator is IDispatcher, Ownable2StepUpgradeable { depositId, uint96(amountToWithdraw) ); + // slither-disable-next-line incorrect-equality } else if (amountToWithdraw == depositBalance) { mezoPortal.withdraw(address(tbtc), depositId); } else {