From 4f67f035076dd6dd37461d14a0fd771bbf9645f1 Mon Sep 17 00:00:00 2001 From: Jakub Date: Fri, 6 Sep 2024 14:44:31 +0200 Subject: [PATCH 1/3] Fix returns in mintDebt and repayDebt functions The functions mintDebt and repayDebt are expected to return the amount of assets calculated based on the provided amount of shares: ``` ) public whenNotPaused returns (uint256 assets) { ``` but they were returning the amount of shares instead: ``` return shares; ``` Here we fix this problem and add tests to cover these cases. --- solidity/contracts/stBTC.sol | 4 ---- solidity/test/stBTC.test.ts | 13 +++++++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/solidity/contracts/stBTC.sol b/solidity/contracts/stBTC.sol index 6da6abf48..e2e0dee46 100644 --- a/solidity/contracts/stBTC.sol +++ b/solidity/contracts/stBTC.sol @@ -326,8 +326,6 @@ contract stBTC is ERC4626Fees, PausableOwnable { // Mint the shares to the receiver. super._mint(receiver, shares); - - return shares; } /// @dev This function proxies `mintDebt` call and provides compatibility @@ -368,8 +366,6 @@ contract stBTC is ERC4626Fees, PausableOwnable { // Burn the shares from the debtor. super._burn(msg.sender, shares); - - return shares; } /// @notice This function proxies `repayDebt` call and provides diff --git a/solidity/test/stBTC.test.ts b/solidity/test/stBTC.test.ts index 6a1c8680f..64e0f3276 100644 --- a/solidity/test/stBTC.test.ts +++ b/solidity/test/stBTC.test.ts @@ -3287,6 +3287,10 @@ describe("stBTC", () => { newShares, ) }) + + it("should return the expected debt in assets", () => { + expect(mintDebtResult).to.be.eq(expectedNewDebt) + }) }) } }) @@ -3496,6 +3500,8 @@ describe("stBTC", () => { let initialTotalDebt: bigint let initialTotalSupply: bigint let initialTotalAssets: bigint + + let repayDebtResult: bigint let tx: ContractTransactionResponse before(async () => { @@ -3506,6 +3512,9 @@ describe("stBTC", () => { initialTotalSupply = await stbtc.totalSupply() initialTotalAssets = await stbtc.totalAssets() + repayDebtResult = await stbtc + .connect(externalMinter) + .repayDebt.staticCall(repaySharesAmount) tx = await stbtc .connect(externalMinter) .repayDebt(repaySharesAmount) @@ -3553,6 +3562,10 @@ describe("stBTC", () => { repaySharesAmount, ) }) + + it("should return the expected debt in assets", () => { + expect(repayDebtResult).to.be.eq(expectedDebtRepayment) + }) } }) }) From 490b1790154e8c7842541f08ea9b1a11097e2437 Mon Sep 17 00:00:00 2001 From: Jakub Date: Fri, 6 Sep 2024 14:53:59 +0200 Subject: [PATCH 2/3] Fix rounding of shares to assets conversions in debt minting Previously, the shares to assets conversion was rounding down, which could lead to a situation where the minted debt was against the vault's interest. Let's imagine a scenario where the vault has a 10% loss, meaning that each share is worth 0.9 assets. If a user mints 1 share, the debt registered due to rounding down would be 0. This can be used as a griefing attack, where a user could mint shares without registering any debt. To fix the issue, we now round up the shares to assets conversion in mintDebt function. Additionally, for the repayDebt function, we keep rounding down the shares to assets to avoid the opposite issue, where a user could repay more debt than they should. It may happen that conversion in `repayDebt` returns `0`, and it should be validated by the caller. Since, the interface used by the Mezo Portal contract `burnReceipt` function doesn't consider the return value of `repayDebt`, we are performing the validation in the `burnReceipt` function. For consistency we added a similar validation in the `mintReceipt` function. --- solidity/contracts/stBTC.sol | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/solidity/contracts/stBTC.sol b/solidity/contracts/stBTC.sol index e2e0dee46..18937d5fe 100644 --- a/solidity/contracts/stBTC.sol +++ b/solidity/contracts/stBTC.sol @@ -125,6 +125,10 @@ contract stBTC is ERC4626Fees, PausableOwnable { /// Reverts if the dispatcher address is the same. error SameDispatcher(); + /// Reverts if a conversion between shares and assets results in zero, which + /// may happen for small amounts division with rounding. + error ConvertedToZero(uint256 shares, uint256 assets); + /// @notice Emitted when the debt allowance of a debtor is insufficient. /// @dev Used in the debt minting function. /// @param debtor Address of the debtor. @@ -305,7 +309,7 @@ contract stBTC is ERC4626Fees, PausableOwnable { uint256 shares, address receiver ) public whenNotPaused returns (uint256 assets) { - assets = convertToAssets(shares); + assets = previewMintDebt(shares); // Increase the debt of the debtor. currentDebt[msg.sender] += assets; @@ -331,10 +335,13 @@ contract stBTC is ERC4626Fees, PausableOwnable { /// @dev This function proxies `mintDebt` call and provides compatibility /// with Mezo IReceiptToken interface. function mintReceipt(address to, uint256 amount) external { - mintDebt(amount, to); + uint256 assets = mintDebt(amount, to); + if (assets == 0) { + revert ConvertedToZero(amount, assets); + } } - /// @notice Repay the asset debt, fully of partially with the provided shares. + /// @notice Repay the asset debt, fully or partially with the provided shares. /// @dev The debt to be repaid is calculated based on the current conversion /// rate from the shares to assets. /// @dev The debtor has to approve the transfer of the shares. To determine @@ -345,7 +352,7 @@ contract stBTC is ERC4626Fees, PausableOwnable { function repayDebt( uint256 shares ) public whenNotPaused returns (uint256 assets) { - assets = convertToAssets(shares); + assets = previewRepayDebt(shares); // Check the current debt of the debtor. if (currentDebt[msg.sender] < assets) { @@ -371,7 +378,10 @@ contract stBTC is ERC4626Fees, PausableOwnable { /// @notice This function proxies `repayDebt` call and provides /// compatibility with Mezo IReceiptToken interface. function burnReceipt(uint256 amount) external { - repayDebt(amount); + uint256 assets = repayDebt(amount); + if (assets == 0) { + revert ConvertedToZero(amount, assets); + } } /// @notice Mints shares to receiver by depositing exactly amount of @@ -524,10 +534,18 @@ contract stBTC is ERC4626Fees, PausableOwnable { return convertToAssets(balanceOf(account)); } - /// @notice Previews the amount of assets that will be burned for the given - /// amount of repaid shares. + /// @notice Previews the amount of assets that will be added to debt when + /// minting the given amount of shares. + /// @dev Rounds the assets up in favor of the vault. + function previewMintDebt(uint256 shares) public view returns (uint256) { + return _convertToAssets(shares, Math.Rounding.Ceil); + } + + /// @notice Previews the amount of assets that will be repaid when returning + /// the given amount of shares. + /// @dev Rounds the assets down in favor of the vault. function previewRepayDebt(uint256 shares) public view returns (uint256) { - return convertToAssets(shares); + return _convertToAssets(shares, Math.Rounding.Floor); } /// @return Returns entry fee basis point used in deposits. From 2f87ecec77e30faf601d3da89794a9a4ce9e29de Mon Sep 17 00:00:00 2001 From: Jakub Date: Fri, 6 Sep 2024 15:09:41 +0200 Subject: [PATCH 3/3] Update unit tests for minting and burning stBTC debt These tests are covering changes from 490b1790154e8c7842541f08ea9b1a11097e2437 We also add tests to verify conversions in case of vault registering a loss. --- solidity/contracts/test/stBTCStub.sol | 14 +++ solidity/deploy/01_deploy_stbtc.ts | 2 +- solidity/test/stBTC.test.ts | 135 ++++++++++++++++++++++---- 3 files changed, 130 insertions(+), 21 deletions(-) create mode 100644 solidity/contracts/test/stBTCStub.sol diff --git a/solidity/contracts/test/stBTCStub.sol b/solidity/contracts/test/stBTCStub.sol new file mode 100644 index 000000000..3a4454654 --- /dev/null +++ b/solidity/contracts/test/stBTCStub.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-3.0-only +pragma solidity 0.8.24; + +import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; + +import "../stBTC.sol"; + +contract stBTCStub is stBTC { + using SafeERC20 for IERC20; + + function workaround_transfer(address to, uint256 amount) external { + IERC20(asset()).safeTransfer(to, amount); + } +} diff --git a/solidity/deploy/01_deploy_stbtc.ts b/solidity/deploy/01_deploy_stbtc.ts index aaedafcdc..a4fa24c81 100644 --- a/solidity/deploy/01_deploy_stbtc.ts +++ b/solidity/deploy/01_deploy_stbtc.ts @@ -10,7 +10,7 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { const tbtc = await deployments.get("TBTC") const [, stbtcDeployment] = await helpers.upgrades.deployProxy("stBTC", { - contractName: "stBTC", + contractName: hre.network.name === "hardhat" ? "stBTCStub" : "stBTC", initializerArgs: [tbtc.address, treasury], factoryOpts: { signer: deployerSigner, diff --git a/solidity/test/stBTC.test.ts b/solidity/test/stBTC.test.ts index 64e0f3276..e0d7cec5c 100644 --- a/solidity/test/stBTC.test.ts +++ b/solidity/test/stBTC.test.ts @@ -3141,9 +3141,7 @@ describe("stBTC", () => { const earnedYield = to1e18(6) // assets = shares * total assets / total supply - // the -1n comes from convertToAssets implementation in - // ERC4626Upgradeable - const expectedDebt = to1e18(15) - 1n + const expectedDebt = to1e18(15) before(async () => { await tbtc.mint(await stbtc.getAddress(), earnedYield) @@ -3155,6 +3153,53 @@ describe("stBTC", () => { expectedDebt, ) }) + + describe("when there is loss generated", () => { + beforeAfterSnapshotWrapper() + + const lossAmount = to1e18(4) + + before(async () => { + await stbtc.workaround_transfer( + thirdParty.address, + lossAmount, + ) + }) + + describe("for big amounts", () => { + beforeAfterSnapshotWrapper() + + // Initial state: + // Deposited assets: 24 + // Debt: 0 + // Loss: 4 + // New mint: + // Shares: 12 + // Expected debt: 12 * (24 - 4) / 24 = 10 + + const sharesAmount = newDebtShares + const expectedDebt = to1e18(10) + 1n + + testMintDebt( + () => sharesOwner1.address, + sharesAmount, + expectedDebt, + ) + }) + + describe("for minimum amount", () => { + beforeAfterSnapshotWrapper() + + const sharesAmount = 1n + const expectedDebt = 1n + + testMintDebt( + () => sharesOwner1.address, + sharesAmount, + expectedDebt, + ) + }) + }) }) }) }) @@ -3225,6 +3270,8 @@ describe("stBTC", () => { let initialTotalDebt: bigint let initialTotalSupply: bigint let initialTotalAssets: bigint + + let mintDebtResult: bigint let tx: ContractTransactionResponse before(async () => { @@ -3236,6 +3283,9 @@ describe("stBTC", () => { initialTotalSupply = await stbtc.totalSupply() initialTotalAssets = await stbtc.totalAssets() + mintDebtResult = await stbtc + .connect(minter) + .mintDebt.staticCall(newShares, receiverAddress) tx = await stbtc .connect(minter) .mintDebt(newShares, receiverAddress) @@ -3470,11 +3520,11 @@ describe("stBTC", () => { // Initial state: // Deposited assets: 24 - // Debt: 10 + // Debt: 12 // Yield: 6 // Repayment: // Shares: 6 - // Expected asset repayment: 6 * (24 + 10 + 6) / (24 + 10) = ~7 + // Expected asset repayment: 6 * (24 + 12 + 6) / (24 + 12) = 7 const earnedYield = to1e18(6) @@ -3488,6 +3538,45 @@ describe("stBTC", () => { testRepayDebt(requestedRepayAmount, expectedDebtRepay) }) + + describe("when there is loss generated", () => { + beforeAfterSnapshotWrapper() + + const lossAmount = to1e18(4) + + before(async () => { + await stbtc.workaround_transfer( + thirdParty.address, + lossAmount, + ) + }) + + describe("for big amounts", () => { + beforeAfterSnapshotWrapper() + + // Initial state: + // Deposited assets: 24 + // Debt: 12 + // Loss: 4 + // Repayment: + // Shares: 6 + // Expected asset repayment: 6 * (24 + 12 - 4) / (24 + 12) ~= 5.33 + + const sharesAmount = requestedRepayAmount + const expectedDebtRepay = 5333333333333333333n + + testRepayDebt(sharesAmount, expectedDebtRepay) + }) + + describe("for minimum amount", () => { + beforeAfterSnapshotWrapper() + + const sharesAmount = 1n + const expectedDebtRepay = 0n + + testRepayDebt(sharesAmount, expectedDebtRepay) + }) + }) }) }) }) @@ -3595,9 +3684,8 @@ describe("stBTC", () => { testRoundTrip( 10n, - 10n, - { totalAssets: 10n, totalSupply: 10n }, - { totalAssets: 0n, totalSupply: 0n }, + { totalDebt: 10n, totalAssets: 10n, totalSupply: 10n }, + { totalDebt: 0n, totalAssets: 0n, totalSupply: 0n }, ) }) @@ -3621,13 +3709,13 @@ describe("stBTC", () => { testRoundTrip( 10n, - 10n, + // totalDebt = 10 * 23 / 23 = 10 // totalAssets = 23 + 10 = 33 // totalSupply = 23 + 10 = 33 - { totalAssets: 33n, totalSupply: 33n }, + { totalDebt: 10n, totalAssets: 33n, totalSupply: 33n }, // totalAssets = 33 - 10 = 23 // totalSupply = 33 - 10 = 23 - { totalAssets: 23n, totalSupply: 23n }, + { totalDebt: 0n, totalAssets: 23n, totalSupply: 23n }, ) }) @@ -3642,26 +3730,29 @@ describe("stBTC", () => { testRoundTrip( 10n, - // 10 * (23 + 5) / 23 = 12 - 12n, - // totalAssets = 23 + 5 + 12 = 40 + // totalDebt = 10 * (23 + 5) / 23 = ceil(12,17) = 13 + // totalAssets = 23 + 5 + 13 = 41 // totalSupply = 23 + 10 = 33 - { totalAssets: 40n, totalSupply: 33n }, - // totalAssets = 40 - 12 = 28 + { totalDebt: 13n, totalAssets: 41n, totalSupply: 33n }, + // assets = 10 * 41 / 33 = floor(12,17) = 12 + // totalDebt = 1 due to rounding + // totalAssets = 41 - 12 = 29 // totalSupply = 33 - 10 = 23 - { totalAssets: 28n, totalSupply: 23n }, + { totalDebt: 1n, totalAssets: 29n, totalSupply: 23n }, ) }) }) function testRoundTrip( shares: bigint, - expectedDebtAmount: bigint, + expectedMintDebtResult: { + totalDebt: bigint totalAssets: bigint totalSupply: bigint }, expectedRepayDebtResults: { + totalDebt: bigint totalAssets: bigint totalSupply: bigint }, @@ -3684,7 +3775,9 @@ describe("stBTC", () => { }) it("should increase total debt", async () => { - expect(await stbtc.totalDebt()).to.be.eq(expectedDebtAmount) + expect(await stbtc.totalDebt()).to.be.eq( + expectedMintDebtResult.totalDebt, + ) }) it("should increase total assets", async () => { @@ -3716,7 +3809,9 @@ describe("stBTC", () => { }) it("should decrease total debt", async () => { - expect(await stbtc.totalDebt()).to.be.eq(0) + expect(await stbtc.totalDebt()).to.be.eq( + expectedRepayDebtResults.totalDebt, + ) }) it("should decrease total assets", async () => {