From d5d2d1e2d1b62ee9fb2fc556ffb790a0aa696d39 Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Tue, 18 Jun 2024 11:58:23 -0400 Subject: [PATCH 1/9] test: move to use setup --- test/InflationAttack.t.sol | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index 924200e..af8e038 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -12,12 +12,16 @@ contract InflationAttackTests is PSMTestBase { // TODO: Add DOS attack test outlined here: https://github.com/marsfoundation/spark-psm/pull/2#pullrequestreview-2085880206 // TODO: Decide if DAI test is needed - function test_inflationAttack_noInitialDeposit() public { - psm = new PSM3(address(dai), address(usdc), address(sDai), address(rateProvider)); + address firstDepositor = makeAddr("firstDepositor"); + address frontRunner = makeAddr("frontRunner"); + address deployer = makeAddr("deployer"); - address firstDepositor = makeAddr("firstDepositor"); - address frontRunner = makeAddr("frontRunner"); + function setUp() public override { + super.setUp(); + psm = new PSM3(address(dai), address(usdc), address(sDai), address(rateProvider)); + } + function test_inflationAttack_noInitialDeposit() public { // Step 1: Front runner deposits 1 sDAI to get 1 share // Have to use sDai because 1 USDC mints 1e12 shares @@ -60,13 +64,7 @@ contract InflationAttackTests is PSMTestBase { } function test_inflationAttack_useInitialDeposit() public { - psm = new PSM3(address(dai), address(usdc), address(sDai), address(rateProvider)); - - address firstDepositor = makeAddr("firstDepositor"); - address frontRunner = makeAddr("frontRunner"); - address deployer = address(this); // TODO: Update to use non-deployer receiver - - _deposit(address(sDai), address(this), 0.8e18); /// 1e18 shares + _deposit(address(sDai), address(deployer), 0.8e18); /// 1e18 shares // Step 1: Front runner deposits sDAI to get 1 share From 671d90f05186b9f45cc4a648fd9971ed176570c3 Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Tue, 18 Jun 2024 12:04:15 -0400 Subject: [PATCH 2/9] test: refactor to add dai tests --- test/InflationAttack.t.sol | 50 +++++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index af8e038..c8d3183 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -21,12 +21,51 @@ contract InflationAttackTests is PSMTestBase { psm = new PSM3(address(dai), address(usdc), address(sDai), address(rateProvider)); } - function test_inflationAttack_noInitialDeposit() public { + function test_inflationAttack_noInitialDeposit_sDai() public { // Step 1: Front runner deposits 1 sDAI to get 1 share // Have to use sDai because 1 USDC mints 1e12 shares _deposit(address(sDai), frontRunner, 1); + _runInflationAttack_noInitialDepositTest(); + } + + function test_inflationAttack_noInitialDeposit_dai() public { + // Step 1: Front runner deposits 1 sDAI to get 1 share + + // Have to use DAI because 1 USDC mints 1e12 shares + _deposit(address(dai), frontRunner, 1); + + _runInflationAttack_noInitialDepositTest(); + } + + function test_inflationAttack_useInitialDeposit_sDai() public { + _deposit(address(sDai), address(deployer), 0.8e18); // 1e18 shares + + // Step 1: Front runner deposits sDAI to get 1 share + + // User tries to do the same attack, depositing one sDAI for 1 share + _deposit(address(sDai), frontRunner, 1); + + assertEq(psm.shares(frontRunner), 1); + + _runInflationAttack_useInitialDepositTest(); + } + + function test_inflationAttack_useInitialDeposit_dai() public { + _deposit(address(dai), address(deployer), 1e18); // 1e18 shares + + // Step 1: Front runner deposits dai to get 1 share + + // User tries to do the same attack, depositing one sDAI for 1 share + _deposit(address(dai), frontRunner, 1); + + assertEq(psm.shares(frontRunner), 1); + + _runInflationAttack_useInitialDepositTest(); + } + + function _runInflationAttack_noInitialDepositTest() internal { assertEq(psm.shares(frontRunner), 1); // Step 2: Front runner transfers 10m USDC to inflate the exchange rate to 1:(10m + 1) @@ -63,14 +102,7 @@ contract InflationAttackTests is PSMTestBase { assertEq(usdc.balanceOf(frontRunner), 15_000_000e6); } - function test_inflationAttack_useInitialDeposit() public { - _deposit(address(sDai), address(deployer), 0.8e18); /// 1e18 shares - - // Step 1: Front runner deposits sDAI to get 1 share - - // User tries to do the same attack, depositing one sDAI for 1 share - _deposit(address(sDai), frontRunner, 1); - + function _runInflationAttack_useInitialDepositTest() internal { assertEq(psm.shares(frontRunner), 1); // Step 2: Front runner transfers 10m USDC to inflate the exchange rate to 1:(10m + 1) From 4785e2fc2bb4863707414e25763607d1c802af37 Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Tue, 18 Jun 2024 12:05:03 -0400 Subject: [PATCH 3/9] fix: rm todo --- test/InflationAttack.t.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index c8d3183..320650c 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -10,7 +10,6 @@ import { PSMTestBase } from "test/PSMTestBase.sol"; contract InflationAttackTests is PSMTestBase { // TODO: Add DOS attack test outlined here: https://github.com/marsfoundation/spark-psm/pull/2#pullrequestreview-2085880206 - // TODO: Decide if DAI test is needed address firstDepositor = makeAddr("firstDepositor"); address frontRunner = makeAddr("frontRunner"); From c2bd83235ae08afdba64c5fe45c30cfb6442093e Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Tue, 18 Jun 2024 12:07:02 -0400 Subject: [PATCH 4/9] fix: rm assertions --- test/InflationAttack.t.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index 320650c..dd2298c 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -46,8 +46,6 @@ contract InflationAttackTests is PSMTestBase { // User tries to do the same attack, depositing one sDAI for 1 share _deposit(address(sDai), frontRunner, 1); - assertEq(psm.shares(frontRunner), 1); - _runInflationAttack_useInitialDepositTest(); } @@ -59,8 +57,6 @@ contract InflationAttackTests is PSMTestBase { // User tries to do the same attack, depositing one sDAI for 1 share _deposit(address(dai), frontRunner, 1); - assertEq(psm.shares(frontRunner), 1); - _runInflationAttack_useInitialDepositTest(); } From f9ffcf264047b584044653c93162c5f107572d82 Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Tue, 18 Jun 2024 15:07:53 -0400 Subject: [PATCH 5/9] fix: rm redundant psm deployment --- test/InflationAttack.t.sol | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index dd2298c..2ba2341 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -15,11 +15,6 @@ contract InflationAttackTests is PSMTestBase { address frontRunner = makeAddr("frontRunner"); address deployer = makeAddr("deployer"); - function setUp() public override { - super.setUp(); - psm = new PSM3(address(dai), address(usdc), address(sDai), address(rateProvider)); - } - function test_inflationAttack_noInitialDeposit_sDai() public { // Step 1: Front runner deposits 1 sDAI to get 1 share From 04656ff24235ea2cf45066114d0eac56d061815d Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Tue, 18 Jun 2024 15:13:34 -0400 Subject: [PATCH 6/9] fix: rm dos attack test --- test/InflationAttack.t.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index 2ba2341..d7ae3ee 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -9,8 +9,6 @@ import { PSMTestBase } from "test/PSMTestBase.sol"; contract InflationAttackTests is PSMTestBase { - // TODO: Add DOS attack test outlined here: https://github.com/marsfoundation/spark-psm/pull/2#pullrequestreview-2085880206 - address firstDepositor = makeAddr("firstDepositor"); address frontRunner = makeAddr("frontRunner"); address deployer = makeAddr("deployer"); From 3f8a4324253644acf7731acec7800734eb10f23d Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Tue, 18 Jun 2024 15:15:45 -0400 Subject: [PATCH 7/9] fix: add back --- test/InflationAttack.t.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index d7ae3ee..2ba2341 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -9,6 +9,8 @@ import { PSMTestBase } from "test/PSMTestBase.sol"; contract InflationAttackTests is PSMTestBase { + // TODO: Add DOS attack test outlined here: https://github.com/marsfoundation/spark-psm/pull/2#pullrequestreview-2085880206 + address firstDepositor = makeAddr("firstDepositor"); address frontRunner = makeAddr("frontRunner"); address deployer = makeAddr("deployer"); From c8efbb3e44e66761b23af54f0460c3f0ecefe982 Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Thu, 20 Jun 2024 09:22:44 -0400 Subject: [PATCH 8/9] fix: merge conflicts --- README.md | 9 +++++++- test/DoSAttack.t.sol | 47 ++++++++++++++++++++++++++++++++++++++ test/InflationAttack.t.sol | 2 -- 3 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 test/DoSAttack.t.sol diff --git a/README.md b/README.md index 00b94ae..dd593b2 100644 --- a/README.md +++ b/README.md @@ -26,7 +26,14 @@ For detailed implementation, refer to the contract code and `IPSM` interface doc ## [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). 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 or DOS attack**. Share inflation attack 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 DOS attack is performed by: +1. Attacker sends funds directly to the PSM. `getPsmTotalValue` now returns a non-zero value. +2. Victim calls deposit. `convertToShares` returns `amount * totalShares / totalValue`. In this case, `totalValue` is non-zero and `totalShares` is zero, so it performs `amount * 0 / totalValue` and returns zero. +3. The victim has `transferFrom` called moving their funds into the PSM, but they receive zero shares so they cannot recover any of their underlying assets. This renders the PSM unusable for all users since this issue will persist. `totalShares` can never be increased in this state. + +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). ## PSM Contract Details diff --git a/test/DoSAttack.t.sol b/test/DoSAttack.t.sol new file mode 100644 index 0000000..7aca07c --- /dev/null +++ b/test/DoSAttack.t.sol @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +pragma solidity ^0.8.13; + +import "forge-std/Test.sol"; + +import { PSMTestBase } from "test/PSMTestBase.sol"; + +contract InflationAttackTests is PSMTestBase { + + address user1 = makeAddr("user1"); + address user2 = makeAddr("user2"); + + function test_dos_sendFundsBeforeFirstDeposit() public { + // Attack pool sending funds in before the first deposit + usdc.mint(address(this), 100e6); + usdc.transfer(address(psm), 100e6); + + assertEq(usdc.balanceOf(address(psm)), 100e6); + + assertEq(psm.totalShares(), 0); + assertEq(psm.shares(user1), 0); + assertEq(psm.shares(user2), 0); + + _deposit(address(usdc), address(user1), 1_000_000e6); + + // Since exchange rate is zero, convertToShares returns 1m * 0 / 100e6 + // because totalValue is not zero so it enters that if statement. + // This results in the funds going in the pool with no way for the user + // to recover them. + assertEq(usdc.balanceOf(address(psm)), 1_000_100e6); + + assertEq(psm.totalShares(), 0); + assertEq(psm.shares(user1), 0); + assertEq(psm.shares(user2), 0); + + // This issue is not related to the first deposit only because totalShares cannot + // get above zero. + _deposit(address(usdc), address(user2), 1_000_000e6); + + assertEq(usdc.balanceOf(address(psm)), 2_000_100e6); + + assertEq(psm.totalShares(), 0); + assertEq(psm.shares(user1), 0); + assertEq(psm.shares(user2), 0); + } + +} diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index 2ba2341..d7ae3ee 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -9,8 +9,6 @@ import { PSMTestBase } from "test/PSMTestBase.sol"; contract InflationAttackTests is PSMTestBase { - // TODO: Add DOS attack test outlined here: https://github.com/marsfoundation/spark-psm/pull/2#pullrequestreview-2085880206 - address firstDepositor = makeAddr("firstDepositor"); address frontRunner = makeAddr("frontRunner"); address deployer = makeAddr("deployer"); From b1a34983a1ee644837b4afa1a044e08e03272884 Mon Sep 17 00:00:00 2001 From: lucas-manuel Date: Thu, 20 Jun 2024 09:23:24 -0400 Subject: [PATCH 9/9] fix: update unused import --- test/InflationAttack.t.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/InflationAttack.t.sol b/test/InflationAttack.t.sol index d7ae3ee..9e69739 100644 --- a/test/InflationAttack.t.sol +++ b/test/InflationAttack.t.sol @@ -3,8 +3,6 @@ pragma solidity ^0.8.13; import "forge-std/Test.sol"; -import { PSM3 } from "src/PSM3.sol"; - import { PSMTestBase } from "test/PSMTestBase.sol"; contract InflationAttackTests is PSMTestBase {