From 44a5d52162acb0acfb749c4c6d0e93b33d300828 Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Jun 2024 09:06:46 -0400 Subject: [PATCH 1/5] feat: update to address comments outside sharesToBurn --- src/PSM.sol | 22 +++++++++++----------- test/Withdraw.t.sol | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/PSM.sol b/src/PSM.sol index d9bd589..72fd270 100644 --- a/src/PSM.sol +++ b/src/PSM.sol @@ -108,11 +108,11 @@ contract PSM { /*** Deposit/withdraw preview functions ***/ /**********************************************************************************************/ - function previewDeposit(address asset, uint256 assets) public view returns (uint256) { + function previewDeposit(address asset, uint256 assetsToDeposit) public view returns (uint256) { require(asset == address(asset0) || asset == address(asset1), "PSM/invalid-asset"); // Convert amount to 1e18 precision denominated in value of asset0 then convert to shares. - return convertToShares(_getAssetValue(asset, assets)); + return convertToShares(_getAssetValue(asset, assetsToDeposit)); } function previewWithdraw(address asset, uint256 maxAssetsToWithdraw) @@ -128,10 +128,10 @@ contract PSM { sharesToBurn = _convertToSharesRoundUp(_getAssetValue(asset, assetsWithdrawn)); - // TODO: Refactor this section to not use convertToAssets because of redundant check - // TODO: Can this cause an underflow in shares? Refactor to use full shares balance? - if (sharesToBurn > shares[msg.sender]) { - assetsWithdrawn = convertToAssets(asset, shares[msg.sender]); + uint256 userShares = shares[msg.sender]; + + if (sharesToBurn > userShares) { + assetsWithdrawn = convertToAssets(asset, userShares); sharesToBurn = _convertToSharesRoundUp(_getAssetValue(asset, assetsWithdrawn)); } } @@ -203,15 +203,15 @@ contract PSM { function _convertToSharesRoundUp(uint256 assetValue) internal view returns (uint256) { uint256 totalValue = getPsmTotalValue(); if (totalValue != 0) { - return _divRoundUp(assetValue * totalShares, totalValue); + return _divUp(assetValue * totalShares, totalValue); } return assetValue; } - function _divRoundUp(uint256 numerator_, uint256 divisor_) - internal pure returns (uint256 result_) - { - result_ = (numerator_ + divisor_ - 1) / divisor_; + function _divUp(uint256 x, uint256 y) internal pure returns (uint256 z) { + unchecked { + z = x != 0 ? ((x - 1) / y) + 1 : 0; + } } function _getAssetValue(address asset, uint256 amount) internal view returns (uint256) { diff --git a/test/Withdraw.t.sol b/test/Withdraw.t.sol index bcabccb..453bb27 100644 --- a/test/Withdraw.t.sol +++ b/test/Withdraw.t.sol @@ -310,7 +310,7 @@ contract PSMWithdrawTests is PSMTestBase { // ); } - function _checkPsmInvariant() internal { + function _checkPsmInvariant() internal view { uint256 totalSharesValue = psm.convertToAssetValue(psm.totalShares()); uint256 totalAssetsValue = sDai.balanceOf(address(psm)) * rateProvider.getConversionRate() / 1e27 From 9c3958e204ee6b325249441bedd54197f0331647 Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Jun 2024 09:18:34 -0400 Subject: [PATCH 2/5] feat: update inflation attack test and readme --- README.md | 2 +- test/InflationAttack.t.sol | 28 ++++++++++++++++------------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index e3336fb..ff69c49 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ PSM contracts to either: ## [CRITICAL]: First Depositor Attack Prevention on Deployment -On the deployment of the PSM, the deployer **MUST make an initial deposit in order to protect the first depositor from getting attacked with a share inflation attack**. This is outlined further [here](https://github.com/marsfoundation/spark-automations/assets/44272939/9472a6d2-0361-48b0-b534-96a0614330d3). 1000 shares minted is determined to be sufficient to prevent this attack. Technical details related to this can be found in `test/InflationAttack.t.sol`. The deployment script [TODO] in this repo contains logic for the deployer to perform this initial deposit, so it is **HIGHLY RECOMMENDED** to use this deployment script when deploying the PSM. Reasoning for the technical implementation approach taken is outlined in more detail [here](https://github.com/marsfoundation/spark-psm/pull/2). +On the deployment of the PSM, the deployer **MUST make an initial deposit to get at least 1e18 shares in order to protect the first depositor from getting attacked with a share inflation attack**. This is outlined further [here](https://github.com/marsfoundation/spark-automations/assets/44272939/9472a6d2-0361-48b0-b534-96a0614330d3). 1e18 shares minted is determined to be sufficient to prevent this attack. Technical details related to this can be found in `test/InflationAttack.t.sol`. The deployment script [TODO] in this repo contains logic for the deployer to perform this initial deposit, so it is **HIGHLY RECOMMENDED** to use this deployment script when deploying the PSM. Reasoning for the technical implementation approach taken is outlined in more detail [here](https://github.com/marsfoundation/spark-psm/pull/2). ## Usage diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index 6c0c047..8cf773f 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -28,6 +28,8 @@ contract InflationAttackTests is PSMTestBase { deal(address(usdc), frontRunner, 10_000_000e6); + assertEq(psm.convertToAssetValue(1), 1); + vm.prank(frontRunner); usdc.transfer(address(psm), 10_000_000e6); @@ -63,9 +65,9 @@ contract InflationAttackTests is PSMTestBase { address frontRunner = makeAddr("frontRunner"); address deployer = address(this); // TODO: Update to use non-deployer receiver - _deposit(address(this), address(sDai), 800); /// 1000 shares + _deposit(address(this), address(sDai), 0.8e18); /// 1e18 shares - // Step 1: Front runner deposits 801 sDAI to get 1 share + // Step 1: Front runner deposits sDAI to get 1 share // User tries to do the same attack, depositing one sDAI for 1 share _deposit(frontRunner, address(sDai), 1); @@ -74,23 +76,25 @@ contract InflationAttackTests is PSMTestBase { // Step 2: Front runner transfers 10m USDC to inflate the exchange rate to 1:(10m + 1) + assertEq(psm.convertToAssetValue(1), 1); + deal(address(usdc), frontRunner, 10_000_000e6); vm.prank(frontRunner); usdc.transfer(address(psm), 10_000_000e6); - // Much less inflated exchange rate - assertEq(psm.convertToAssetValue(1), 9990.009990009990009991e18); + // Still inflated, but all value is transferred to existing holder, deployer + assertEq(psm.convertToAssetValue(1), 0.00000000001e18); - // Step 3: First depositor deposits 20 million USDC, only gets one share because rounding - // error gives them 1 instead of 2 shares, worth 15m USDC + // Step 3: First depositor deposits 20 million USDC, this time rounding is not an issue + // so value reflected is much more accurate _deposit(firstDepositor, address(usdc), 20_000_000e6); - assertEq(psm.shares(firstDepositor), 2001); + assertEq(psm.shares(firstDepositor), 1.999999800000020001e18); // Higher amount of initial shares means lower rounding error - assertEq(psm.convertToAssetValue(2001), 19_996_668.887408394403731513e18); + assertEq(psm.convertToAssetValue(1.999999800000020001e18), 19_999_999.999999999996673334e18); // Step 4: Both users withdraw the max amount of funds they can @@ -98,10 +102,10 @@ contract InflationAttackTests is PSMTestBase { _withdraw(frontRunner, address(usdc), type(uint256).max); _withdraw(deployer, address(usdc), type(uint256).max); - // Front runner loses 9.99m USDC, first depositor loses 4k USDC - assertEq(usdc.balanceOf(firstDepositor), 19_996_668.887408e6); - assertEq(usdc.balanceOf(frontRunner), 9_993.337774e6); - assertEq(usdc.balanceOf(deployer), 9_993_337.774818e6); + // Front runner loses full 10m USDC to the deployer that had all shares at the beginning, first depositor loses nothing (1e-6 USDC) + assertEq(usdc.balanceOf(firstDepositor), 19_999_999.999999e6); + assertEq(usdc.balanceOf(frontRunner), 0); + assertEq(usdc.balanceOf(deployer), 10_000_000.000001e6); } } From 4c9cf09e2bdbd87b8a60d7f3b5b9935d2921b81d Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Jun 2024 09:19:31 -0400 Subject: [PATCH 3/5] fix: update readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index ff69c49..f7d0ed8 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ PSM contracts to either: ## [CRITICAL]: First Depositor Attack Prevention on Deployment -On the deployment of the PSM, the deployer **MUST make an initial deposit to get at least 1e18 shares in order to protect the first depositor from getting attacked with a share inflation attack**. This is outlined further [here](https://github.com/marsfoundation/spark-automations/assets/44272939/9472a6d2-0361-48b0-b534-96a0614330d3). 1e18 shares minted is determined to be sufficient to prevent this attack. Technical details related to this can be found in `test/InflationAttack.t.sol`. The deployment script [TODO] in this repo contains logic for the deployer to perform this initial deposit, so it is **HIGHLY RECOMMENDED** to use this deployment script when deploying the PSM. Reasoning for the technical implementation approach taken is outlined in more detail [here](https://github.com/marsfoundation/spark-psm/pull/2). +On the deployment of the PSM, the deployer **MUST make an initial deposit to get AT LEAST 1e18 shares in order to protect the first depositor from getting attacked with a share inflation attack**. This is outlined further [here](https://github.com/marsfoundation/spark-automations/assets/44272939/9472a6d2-0361-48b0-b534-96a0614330d3). Technical details related to this can be found in `test/InflationAttack.t.sol`. The deployment script [TODO] in this repo contains logic for the deployer to perform this initial deposit, so it is **HIGHLY RECOMMENDED** to use this deployment script when deploying the PSM. Reasoning for the technical implementation approach taken is outlined in more detail [here](https://github.com/marsfoundation/spark-psm/pull/2). ## Usage From cb9931ad4d3ce00ccfe22ea55469b183af8f598b Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Jun 2024 09:27:22 -0400 Subject: [PATCH 4/5] feat: update test to constrain deposit/withdraw --- src/PSM.sol | 2 +- test/Withdraw.t.sol | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/PSM.sol b/src/PSM.sol index 72fd270..d3c13dd 100644 --- a/src/PSM.sol +++ b/src/PSM.sol @@ -132,7 +132,7 @@ contract PSM { if (sharesToBurn > userShares) { assetsWithdrawn = convertToAssets(asset, userShares); - sharesToBurn = _convertToSharesRoundUp(_getAssetValue(asset, assetsWithdrawn)); + sharesToBurn = userShares; } } diff --git a/test/Withdraw.t.sol b/test/Withdraw.t.sol index 453bb27..28b6731 100644 --- a/test/Withdraw.t.sol +++ b/test/Withdraw.t.sol @@ -168,7 +168,11 @@ contract PSMWithdrawTests is PSMTestBase { assertEq(psm.shares(user2), 0); // Burns the users full amount of shares } - function testFuzz_withdraw_multiUser( + // Adding this test to demonstrate that numbers are exact and correspond to assets deposits/withdrawals when withdrawals + // aren't greater than the user's share balance. The next test doesn't constrain this, but there are rounding errors of + // up to 1e12 for USDC because of the difference in asset precision. Up to 1e12 shares can be burned for 0 USDC in some + // cases, but this is an intentional rounding error against the user. + function testFuzz_withdraw_multiUser_noFullShareBurns( uint256 depositAmount1, uint256 depositAmount2, uint256 depositAmount3, @@ -184,9 +188,9 @@ contract PSMWithdrawTests is PSMTestBase { depositAmount2 = bound(depositAmount2, 0, USDC_TOKEN_MAX); depositAmount3 = bound(depositAmount3, 0, SDAI_TOKEN_MAX); - withdrawAmount1 = bound(withdrawAmount1, 0, USDC_TOKEN_MAX); - withdrawAmount2 = bound(withdrawAmount2, 0, USDC_TOKEN_MAX); - withdrawAmount3 = bound(withdrawAmount3, 0, SDAI_TOKEN_MAX); + withdrawAmount1 = bound(withdrawAmount1, depositAmount1, USDC_TOKEN_MAX); + withdrawAmount2 = bound(withdrawAmount2, depositAmount2, USDC_TOKEN_MAX); + withdrawAmount3 = bound(withdrawAmount3, 0, SDAI_TOKEN_MAX); _deposit(user1, address(usdc), depositAmount1); _deposit(user2, address(usdc), depositAmount2); From e576672c71e6043f7ce806d3adb356f0901f9398 Mon Sep 17 00:00:00 2001 From: Lucas Date: Mon, 3 Jun 2024 09:45:55 -0400 Subject: [PATCH 5/5] feat: update to add both cases --- test/Withdraw.t.sol | 92 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 79 insertions(+), 13 deletions(-) diff --git a/test/Withdraw.t.sol b/test/Withdraw.t.sol index 28b6731..66fe2cb 100644 --- a/test/Withdraw.t.sol +++ b/test/Withdraw.t.sol @@ -188,10 +188,68 @@ contract PSMWithdrawTests is PSMTestBase { depositAmount2 = bound(depositAmount2, 0, USDC_TOKEN_MAX); depositAmount3 = bound(depositAmount3, 0, SDAI_TOKEN_MAX); - withdrawAmount1 = bound(withdrawAmount1, depositAmount1, USDC_TOKEN_MAX); - withdrawAmount2 = bound(withdrawAmount2, depositAmount2, USDC_TOKEN_MAX); - withdrawAmount3 = bound(withdrawAmount3, 0, SDAI_TOKEN_MAX); + withdrawAmount1 = bound(withdrawAmount1, 0, USDC_TOKEN_MAX); + withdrawAmount2 = bound(withdrawAmount2, 0, depositAmount2); // User can't burn up to 1e12 shares for 0 USDC in this case + withdrawAmount3 = bound(withdrawAmount3, 0, SDAI_TOKEN_MAX); + + // Run with zero share tolerance because the rounding error shouldn't be introduced with the above constraints. + _runWithdrawFuzzTests( + 0, + depositAmount1, + depositAmount2, + depositAmount3, + withdrawAmount1, + withdrawAmount2, + withdrawAmount3 + ); + } + + function testFuzz_withdraw_multiUser_fullShareBurns( + uint256 depositAmount1, + uint256 depositAmount2, + uint256 depositAmount3, + uint256 withdrawAmount1, + uint256 withdrawAmount2, + uint256 withdrawAmount3 + ) + public + { + // NOTE: Not covering zero cases, 1e-2 at 1e6 used as min for now so exact values can + // be asserted + depositAmount1 = bound(depositAmount1, 0, USDC_TOKEN_MAX); + depositAmount2 = bound(depositAmount2, 0, USDC_TOKEN_MAX); + depositAmount3 = bound(depositAmount3, 0, SDAI_TOKEN_MAX); + withdrawAmount1 = bound(withdrawAmount1, 0, USDC_TOKEN_MAX); + withdrawAmount2 = bound(withdrawAmount2, 0, USDC_TOKEN_MAX); + withdrawAmount3 = bound(withdrawAmount3, 0, SDAI_TOKEN_MAX); + + // Run with 1e12 share tolerance because the rounding error will be introduced with the above constraints. + _runWithdrawFuzzTests( + 1e12, + depositAmount1, + depositAmount2, + depositAmount3, + withdrawAmount1, + withdrawAmount2, + withdrawAmount3 + ); + } + + // NOTE: For `assertApproxEqAbs` assertions, a difference calculation is used here instead of comparing + // the two values because this approach inherently asserts that the shares remaining are lower than the + // theoretical value, proving the PSM rounds agains the user. + function _runWithdrawFuzzTests( + uint256 usdcShareTolerance, + uint256 depositAmount1, + uint256 depositAmount2, + uint256 depositAmount3, + uint256 withdrawAmount1, + uint256 withdrawAmount2, + uint256 withdrawAmount3 + ) + internal + { _deposit(user1, address(usdc), depositAmount1); _deposit(user2, address(usdc), depositAmount2); _deposit(user2, address(sDai), depositAmount3); @@ -220,6 +278,9 @@ contract PSMWithdrawTests is PSMTestBase { totalValue ); + // NOTE: User 1 doesn't need a tolerance because their shares are 1e6 precision because they only + // deposited USDC. User 2 has a tolerance because they deposited sDAI which has 1e18 precision + // so there is a chance that the rounding will be off by up to 1e12. assertEq(usdc.balanceOf(user1), expectedWithdrawnAmount1); assertEq(usdc.balanceOf(user2), 0); assertEq(usdc.balanceOf(address(psm)), totalUsdc - expectedWithdrawnAmount1); @@ -252,12 +313,17 @@ contract PSMWithdrawTests is PSMTestBase { assertEq(psm.shares(user1), (depositAmount1 - expectedWithdrawnAmount1) * 1e12); - assertEq( - psm.shares(user2), - (depositAmount2 * 1e12) + (depositAmount3 * 125/100) - (expectedWithdrawnAmount2 * 1e12) + assertApproxEqAbs( + ((depositAmount2 * 1e12) + (depositAmount3 * 125/100) - (expectedWithdrawnAmount2 * 1e12)) - psm.shares(user2), + 0, + usdcShareTolerance ); - assertEq(psm.totalShares(), totalValue - (expectedWithdrawnAmount1 + expectedWithdrawnAmount2) * 1e12); + assertApproxEqAbs( + (totalValue - (expectedWithdrawnAmount1 + expectedWithdrawnAmount2) * 1e12) - psm.totalShares(), + 0, + usdcShareTolerance + ); uint256 expectedWithdrawnAmount3 = _getExpectedWithdrawnAmount(sDai, user2, withdrawAmount3); @@ -287,15 +353,15 @@ contract PSMWithdrawTests is PSMTestBase { assertEq(psm.shares(user1), (depositAmount1 - expectedWithdrawnAmount1) * 1e12); assertApproxEqAbs( - psm.shares(user2), - (depositAmount2 * 1e12) + (depositAmount3 * 125/100) - (expectedWithdrawnAmount2 * 1e12) - (expectedWithdrawnAmount3 * 125/100), - 1 + ((depositAmount2 * 1e12) + (depositAmount3 * 125/100) - (expectedWithdrawnAmount2 * 1e12) - (expectedWithdrawnAmount3 * 125/100)) - psm.shares(user2), + 0, + usdcShareTolerance + 1 // 1 is added to the tolerance because of rounding error in sDAI calculations ); assertApproxEqAbs( - psm.totalShares(), - totalValue - (expectedWithdrawnAmount1 + expectedWithdrawnAmount2) * 1e12 - (expectedWithdrawnAmount3 * 125/100), - 1 + totalValue - (expectedWithdrawnAmount1 + expectedWithdrawnAmount2) * 1e12 - (expectedWithdrawnAmount3 * 125/100) - psm.totalShares(), + 0, + usdcShareTolerance + 1 // 1 is added to the tolerance because of rounding error in sDAI calculations ); // -- TODO: Get these to work, rounding assertions proving always rounding down