Skip to content

Commit

Permalink
Revert on remove collateral if not enough LPs. Mul before div (#613)
Browse files Browse the repository at this point in the history 
- ERC20Pool.removeCollateral should revert with InsufficientLPs error if the redeemed collateral amount is 0.
- Add test to expose swap scenario with small amounts
- Mul before div when calculating amount of collateral to remove
  • Loading branch information
@grandizzy
grandizzy authored Feb 17, 2023
1 parent 69aa084 commit 5752259
Show file tree
Hide file tree
Showing 4 changed files with 279 additions and 3 deletions.
4 changes: 3 additions & 1 deletion src/libraries/external/LenderActions.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,9 @@ library LenderActions {
lpAmount_ = requiredLPs;
} else {
lpAmount_ = lenderLpBalance;
collateralAmount_ = Maths.wmul(Maths.wdiv(lenderLpBalance, requiredLPs), collateralAmount_);
collateralAmount_ = Maths.wdiv(Maths.wmul(lenderLpBalance, collateralAmount_), requiredLPs);

if (collateralAmount_ == 0) revert InsufficientLPs();
}

// update bucket LPs and collateral balance
Expand Down
9 changes: 9 additions & 0 deletions tests/forge/ERC20Pool/ERC20DSTestPlus.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,15 @@ abstract contract ERC20DSTestPlus is DSTestPlus, IERC20PoolEvents {
ERC20Pool(address(_pool)).removeCollateral(amount, index);
}

function _assertRemoveAllCollateralInsufficientLPsRevert(
address from,
uint256 index
) internal {
changePrank(from);
vm.expectRevert(IPoolErrors.InsufficientLPs.selector);
ERC20Pool(address(_pool)).removeCollateral(type(uint256).max, index);
}

function _assertTransferInvalidIndexRevert(
address operator,
address from,
Expand Down
265 changes: 265 additions & 0 deletions tests/forge/ERC20Pool/ERC20PoolCollateral.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -740,4 +740,269 @@ contract ERC20PoolCollateralTest is ERC20HelperContract {
exchangeRate: 1 * 1e18 // exchange rate should not change
});
}

function testAddRemoveCollateralSmallAmountsBucketExchangeRateInvariantDifferentActor() external tearDown {
_mintCollateralAndApproveTokens(_lender, 50000000000 * 1e18);

_addInitialLiquidity({
from: _bidder,
amount: 304,
index: 2570
});

_assertLenderLpBalance({
lender: _lender,
index: 2570,
lpBalance: 0,
depositTime: 0
});
_assertLenderLpBalance({
lender: _bidder,
index: 2570,
lpBalance: 304,
depositTime: _startTime
});
_assertBucket({
index: 2570,
lpBalance: 304,
collateral: 0,
deposit: 304,
exchangeRate: 1 * 1e18 // exchange rate should not change
});

_addCollateral({
from: _lender,
amount: 1,
index: 2570,
lpAward: 2725
});

_assertLenderLpBalance({
lender: _lender,
index: 2570,
lpBalance: 2725,
depositTime: _startTime
});
_assertLenderLpBalance({
lender: _bidder,
index: 2570,
lpBalance: 304,
depositTime: _startTime
});
_assertBucket({
index: 2570,
lpBalance: 3029,
collateral: 1,
deposit: 304,
exchangeRate: 1 * 1e18 // exchange rate should not change
});

// lender should not be able to remove any collateral as LP balance is 304 < 2725
_assertRemoveAllCollateralInsufficientLPsRevert({
from: _bidder,
index: 2570
});

_removeAllCollateral({
from: _lender,
amount: 1,
index: 2570,
lpRedeem: 2725
});

_assertLenderLpBalance({
lender: _lender,
index: 2570,
lpBalance: 0,
depositTime: _startTime
});
_assertLenderLpBalance({
lender: _bidder,
index: 2570,
lpBalance: 304,
depositTime: _startTime
});
_assertBucket({
index: 2570,
lpBalance: 304,
collateral: 0,
deposit: 304,
exchangeRate: 1 * 1e18 // exchange rate should not change
});
}

function testSwapSmallAmountsBucketExchangeRateInvariantDifferentActor() external tearDown {
_mintCollateralAndApproveTokens(_lender, 50000000000 * 1e18);

_addInitialLiquidity({
from: _bidder,
amount: 2725,
index: 2570
});

_assertLenderLpBalance({
lender: _lender,
index: 2570,
lpBalance: 0,
depositTime: 0
});
_assertLenderLpBalance({
lender: _bidder,
index: 2570,
lpBalance: 2725,
depositTime: _startTime
});
_assertBucket({
index: 2570,
lpBalance: 2725,
collateral: 0,
deposit: 2725,
exchangeRate: 1 * 1e18 // exchange rate should not change
});

_addCollateral({
from: _lender,
amount: 1,
index: 2570,
lpAward: 2725
});

_assertLenderLpBalance({
lender: _lender,
index: 2570,
lpBalance: 2725,
depositTime: _startTime
});
_assertLenderLpBalance({
lender: _bidder,
index: 2570,
lpBalance: 2725,
depositTime: _startTime
});
_assertBucket({
index: 2570,
lpBalance: 5450,
collateral: 1,
deposit: 2725,
exchangeRate: 1 * 1e18 // exchange rate should not change
});

uint256 snapshot = vm.snapshot();

// bucket should be cleaned out if collateral swap happens first
_removeAllCollateral({
from: _bidder,
amount: 1,
index: 2570,
lpRedeem: 2725
});
_removeAllLiquidity({
from: _lender,
amount: 2722,
index: 2570,
newLup: MAX_PRICE,
lpRedeem: 2725
});

_assertLenderLpBalance({
lender: _lender,
index: 2570,
lpBalance: 0,
depositTime: _startTime
});
_assertLenderLpBalance({
lender: _bidder,
index: 2570,
lpBalance: 0,
depositTime: _startTime
});
_assertBucket({
index: 2570,
lpBalance: 0,
collateral: 0,
deposit: 0,
exchangeRate: 1 * 1e18 // exchange rate should not change
});

vm.revertTo(snapshot);

// bucket should be cleaned out if quote token swap happens first
_removeAllLiquidity({
from: _lender,
amount: 2722,
index: 2570,
newLup: MAX_PRICE,
lpRedeem: 2725
});
_removeAllCollateral({
from: _bidder,
amount: 1,
index: 2570,
lpRedeem: 2725
});

_assertLenderLpBalance({
lender: _lender,
index: 2570,
lpBalance: 0,
depositTime: _startTime
});
_assertLenderLpBalance({
lender: _bidder,
index: 2570,
lpBalance: 0,
depositTime: _startTime
});
_assertBucket({
index: 2570,
lpBalance: 0,
collateral: 0,
deposit: 0,
exchangeRate: 1 * 1e18 // exchange rate should not change
});
}

function testAddRemoveCollateralBucketExchangeRateInvariantDifferentActor2() external tearDown {
_mintCollateralAndApproveTokens(_lender, 1000000000000000000 * 1e18);
_mintCollateralAndApproveTokens(_bidder, 50000000000 * 1e18);

_addCollateral({
from: _bidder,
amount: 15200,
index: 2570,
lpAward: 41420710
});
_addInitialLiquidity({
from: _lender,
amount: 2,
index: 2570
});
_addCollateral({
from: _lender,
amount: 883976901103343226.563974622543668416 * 1e18,
index: 2570,
lpAward: 2408878317819033617340.926215832879088040 * 1e18
});
_assertLenderLpBalance({
lender: _lender,
index: 2570,
lpBalance: 2408878317819033617340.926215832879088042 * 1e18,
depositTime: _startTime
});

_removeAllCollateral({
from: _lender,
amount: 883976901103343226.563974622543668416 * 1e18,
index: 2570,
lpRedeem: 2408878317819033617340.926215832879088042 * 1e18
});
_assertBucket({
index: 2570,
lpBalance: 41420710,
collateral: 15200,
deposit: 2,
exchangeRate: 1.000000048285024569 * 1e18
});

}
}
4 changes: 2 additions & 2 deletions tests/forge/ERC20Pool/ERC20PoolPurchaseQuote.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ contract ERC20PoolPurchaseQuoteTokenTest is ERC20HelperContract {
});

// bidder withdraws unused collateral
uint256 expectedCollateral = 0.066443194797165079 * 1e18;
uint256 expectedCollateral = 0.066443194797165080 * 1e18;

_removeAllCollateral({
from: _bidder,
Expand All @@ -279,7 +279,7 @@ contract ERC20PoolPurchaseQuoteTokenTest is ERC20HelperContract {
skip(7200);

// lender exchanges their LP for collateral
expectedCollateral = 1.992953578100502458 * 1e18;
expectedCollateral = 1.992953578100502457 * 1e18;

_removeAllCollateral({
from: _lender,
Expand Down

0 comments on commit 5752259

Please sign in to comment.