Skip to content

Commit

Permalink
fix: merge conflicts
Browse files Browse the repository at this point in the history
  • Loading branch information
Lucas committed Jun 3, 2024
2 parents e6c654d + e994287 commit 9688892
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 33 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ 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 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). 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).

## PSM Contract Details

Expand Down
22 changes: 14 additions & 8 deletions src/PSM.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,11 @@ contract PSM is IPSM {
/*** Deposit/withdraw preview functions ***/
/**********************************************************************************************/

function previewDeposit(address asset, uint256 assets) public view override returns (uint256) {
function previewDeposit(address asset, uint256 assetsToDeposit) public view returns (uint256) {
require(_isValidAsset(asset), "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)
Expand All @@ -137,11 +137,11 @@ contract PSM is IPSM {

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]);
sharesToBurn = _convertToSharesRoundUp(_getAssetValue(asset, assetsWithdrawn));
uint256 userShares = shares[msg.sender];

if (sharesToBurn > userShares) {
assetsWithdrawn = convertToAssets(asset, userShares);
sharesToBurn = userShares;
}
}

Expand Down Expand Up @@ -230,11 +230,17 @@ contract PSM is IPSM {
function _convertToSharesRoundUp(uint256 assetValue) internal view returns (uint256) {
uint256 totalValue = getPsmTotalValue();
if (totalValue != 0) {
return ((assetValue * totalShares) + totalValue - 1) / totalValue;
return _divUp(assetValue * totalShares, totalValue);
}
return assetValue;
}

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) {
if (asset == address(asset0)) return _getAsset0Value(amount);
else if (asset == address(asset1)) return _getAsset1Value(amount);
Expand Down
28 changes: 16 additions & 12 deletions test/InflationAttack.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,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);

Expand Down Expand Up @@ -64,9 +66,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);
Expand All @@ -75,34 +77,36 @@ 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

_withdraw(firstDepositor, address(usdc), type(uint256).max);
_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);
}

}
94 changes: 82 additions & 12 deletions test/Withdraw.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,43 @@ 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,
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, 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,
Expand All @@ -188,6 +224,32 @@ contract PSMWithdrawTests is PSMTestBase {
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);
Expand Down Expand Up @@ -216,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);
Expand Down Expand Up @@ -248,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);
Expand Down Expand Up @@ -283,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
Expand All @@ -310,7 +380,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
Expand Down

0 comments on commit 9688892

Please sign in to comment.