fix dust surplus claim in recovery mode liquidation #787
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -969,4 +969,47 @@ contract CdpManagerLiquidationTest is eBTCBaseInvariants { | |
|
|
||
| return (cdpIds, _newPrice); | ||
| } | ||
|
|
||
| function testSurplusInRMWhenICRBelowMCR() public { | ||
| address wallet = users[0]; | ||
|
|
||
| // set eth per stETH share | ||
| collateral.setEthPerShare(1158379174506084879); | ||
|
|
||
| // fetch price before open | ||
| uint256 oldprice = priceFeedMock.fetchPrice(); | ||
|
|
||
| // open five cdps | ||
| _openTestCDP(wallet, 2e18 + 2e17, ((2e18 * oldprice) / 240e16)); | ||
| _openTestCDP(wallet, 2e18 + 2e17, ((2e18 * oldprice) / 240e16)); | ||
| _openTestCDP(wallet, 2e18 + 2e17, ((2e18 * oldprice) / 240e16)); | ||
| _openTestCDP(wallet, 2e18 + 2e17, ((2e18 * oldprice) / 240e16)); | ||
| bytes32 underwater = _openTestCDP(wallet, 2e18 + 2e17, ((2e18 * oldprice) / 210e16)); | ||
|
|
||
| // reduce the price by half to make underwater cdp | ||
| priceFeedMock.setPrice(oldprice / 2); | ||
|
|
||
| // fetch new price after reduce | ||
| uint256 newPrice = priceFeedMock.fetchPrice(); | ||
|
|
||
| // ensure the system is in recovery mode | ||
| assert(cdpManager.getSyncedTCR(newPrice) < CCR); | ||
|
|
||
| // liquidate underwater cdp with ICR < MCR | ||
| vm.startPrank(wallet); | ||
| cdpManager.liquidate(underwater); | ||
| vm.stopPrank(); | ||
|
|
||
| // make sure the cdp is no longer in the sorted list | ||
| assert(!sortedCdps.contains(underwater)); | ||
|
|
||
| // fetch the surplus after the liquidation | ||
| uint256 surplus = collSurplusPool.getSurplusCollShares(wallet); | ||
|
|
||
| // console log the surplus coll | ||
| console.log("Surplus:", surplus); | ||
|
|
||
| // ensure that the surplus is zero | ||
| assert(surplus == 0); | ||
| } | ||
|
Comment on lines
+972
to
+1014
There was a problem hiding this comment. The newly added test function
Overall, the addition of this test function is a positive step towards ensuring the system behaves as expected under specific liquidation scenarios in recovery mode. Addressing the above points can further enhance the quality and coverage of the test suite. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,13 +25,16 @@ contract EToFoundry is | |
| { | ||
| modifier setup() override { | ||
| _; | ||
| address sender = uint160(msg.sender) % 3 == 0 ? address(USER1) : uint160(msg.sender) % 3 == 1 | ||
| ? address(USER2) | ||
| : address(USER3); | ||
| actor = actors[sender]; | ||
| } | ||
|
|
||
| function setUp() public { | ||
| _setUp(); | ||
| _setUpActors(); | ||
| actor = actors[address(USER1)]; | ||
| } | ||
|
|
||
| function _checkTotals() internal { | ||
|
Comment on lines
25
to
40
There was a problem hiding this comment.
AWS Access Key ID Value detected in a comment. This is a sensitive credential and should not be hardcoded here. Even though it's in a comment, it's best practice to avoid including potentially sensitive information in your codebase. Consider removing or obfuscating this information.
AWS Access Key ID Value detected in a comment. This is a sensitive credential and should not be hardcoded here. Even though it's in a comment, it's best practice to avoid including potentially sensitive information in your codebase. Consider removing or obfuscating this information.
AWS Access Key ID Value detected in a comment. This is a sensitive credential and should not be hardcoded here. Even though it's in a comment, it's best practice to avoid including potentially sensitive information in your codebase. Consider removing or obfuscating this information.
AWS Access Key ID Value detected in a comment. This is a sensitive credential and should not be hardcoded here. Even though it's in a comment, it's best practice to avoid including potentially sensitive information in your codebase. Consider removing or obfuscating this information. |
||
|
|
@@ -221,9 +224,17 @@ contract EToFoundry is | |
| function _logStakes() internal { | ||
| bytes32 currentCdp = sortedCdps.getFirst(); | ||
|
|
||
| console2.log("=== LogStakes ==="); | ||
|
|
||
| uint256 currentPrice = priceFeedMock.fetchPrice(); | ||
| uint256 currentPricePerShare = collateral.getPooledEthByShares(1 ether); | ||
| console2.log("currentPrice", currentPrice); | ||
| console2.log("currentPricePerShare", currentPricePerShare); | ||
|
|
||
| while (currentCdp != bytes32(0)) { | ||
| emit DebugBytes32(currentCdp); | ||
| console2.log("CdpId", vm.toString(currentCdp)); | ||
| console2.log("==============================="); | ||
| console2.log("cdpManager.getCdpStake(currentCdp)", cdpManager.getCdpStake(currentCdp)); | ||
| console2.log( | ||
| "cdpManager.getSyncedCdpCollShares(currentCdp)", | ||
|
|
@@ -239,7 +250,16 @@ contract EToFoundry is | |
| "cdpManager.getSyncedNominalICR(currentCdp)", | ||
| cdpManager.getSyncedNominalICR(currentCdp) | ||
| ); | ||
| console2.log( | ||
| "cdpManager.getCachedICR(currentCdp, currentPrice)", | ||
| cdpManager.getCachedICR(currentCdp, currentPrice) | ||
| ); | ||
| console2.log( | ||
| "cdpManager.getSyncedICR(currentCdp, currentPrice)", | ||
| cdpManager.getSyncedICR(currentCdp, currentPrice) | ||
| ); | ||
| currentCdp = sortedCdps.getNext(currentCdp); | ||
| console2.log(""); | ||
| } | ||
|
|
||
| console2.log( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,12 @@ import "../contracts/Interfaces/IERC3156FlashLender.sol"; | |
| * FlashLoan ReEntrancy Attack | ||
| */ | ||
|
|
||
| interface IActivePool { | ||
| function feeRecipientAddress() external view returns (address); | ||
|
|
||
| function feeBps() external view returns (uint256); | ||
| } | ||
|
|
||
| contract FlashAttack { | ||
| IERC20 public immutable want; | ||
| IERC3156FlashLender public immutable lender; | ||
|
|
@@ -45,6 +51,37 @@ contract FlashAttack { | |
| } | ||
| } | ||
|
|
||
| contract FlashFeeEscapeBorrower { | ||
| IERC20 public immutable want; | ||
| IERC3156FlashLender public immutable lender; | ||
| uint256 public counter; | ||
|
|
||
| constructor(IERC20 _want, IERC3156FlashLender _lender, uint256 _amt) { | ||
| want = _want; | ||
| lender = _lender; | ||
|
|
||
| // Approve to repay | ||
| IERC20(_want).approve(address(_lender), type(uint256).max); | ||
| counter = _amt; | ||
| } | ||
|
|
||
| function onFlashLoan( | ||
| address initiator, | ||
| address token, | ||
| uint256 amount, | ||
| uint256 fee, | ||
| bytes calldata data | ||
| ) external returns (bytes32) { | ||
| require(token == address(want)); | ||
|
|
||
| if (IERC20(want).balanceOf(address(this)) < counter) { | ||
| lender.flashLoan(IERC3156FlashBorrower(address(this)), address(want), amount, data); | ||
| } | ||
|
|
||
| return keccak256("ERC3156FlashBorrower.onFlashLoan"); | ||
| } | ||
| } | ||
|
Comment on lines
+54
to
+83
There was a problem hiding this comment. The Consider adding reentrancy guards to the |
||
|
|
||
| contract FlashLoanAttack is eBTCBaseFixture { | ||
| function setUp() public override { | ||
| // Base setup | ||
|
|
@@ -160,4 +197,33 @@ contract FlashLoanAttack is eBTCBaseFixture { | |
| abi.encodePacked(uint256(0)) | ||
| ); | ||
| } | ||
|
|
||
| function testFeeEscapeAttack() public { | ||
| uint256 _feeBps = IActivePool(address(activePool)).feeBps(); | ||
| uint256 amount = 10000 / IActivePool(address(activePool)).feeBps(); | ||
| uint256 totalAmount = amount * _feeBps; | ||
|
|
||
| uint256 fee = activePool.flashFee(address(collateral), amount); | ||
|
|
||
| vm.assume(fee == 0); | ||
|
|
||
| FlashFeeEscapeBorrower attacker = new FlashFeeEscapeBorrower( | ||
| IERC20(address(collateral)), | ||
| IERC3156FlashLender(address(activePool)), | ||
| amount | ||
| ); | ||
|
|
||
| dealCollateral(address(activePool), totalAmount * 2); | ||
|
|
||
| address _feeRecipient = IActivePool(address(activePool)).feeRecipientAddress(); | ||
| uint256 _feeBefore = IERC20(address(collateral)).balanceOf(_feeRecipient); | ||
| activePool.flashLoan( | ||
| IERC3156FlashBorrower(address(attacker)), | ||
| address(collateral), | ||
| amount, | ||
| abi.encodePacked(amount) | ||
| ); | ||
| uint256 _feeAfter = IERC20(address(collateral)).balanceOf(_feeRecipient); | ||
| require(_feeAfter == _feeBefore, "!flash fee should be zero due to division loss"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to
CdpManager.liquidatewithin theliquidatefunction does not explicitly check for the success of the operation before proceeding with further logic. While thesuccessvariable is used later to conditionally execute code, it's crucial to handle the failure case immediately after the external call to prevent any unintended behavior. Consider adding a require statement to ensure that the liquidation call was successful before continuing.(success, returnData) = actor.proxy( address(cdpManager), abi.encodeWithSelector(CdpManager.liquidate.selector, _cdpId) ); + require(success, "Liquidation failed");Committable suggestion