Alice can front-run a liquidator call liquidate(alice) by calling depositCollateralAndIncreaseShare with a value > DUST and DoS/prevent its liquidation, as liquidate is limited by cooldownExpiration in _decreaseUserShare
Why?
1) every time _increaseUserShare or _decreaseUserShare is called with useCooldown = true, a cooldown is set to block.timestamp + modificationCooldown. This cooldown will prevent any calls to these functions (if useCooldown = true) until the time has passed
2) Because depositCollateralAndIncreaseShare (which calls _depositLiquidityAndIncreaseShare, itself calling _increaseUserShare with useCooldown == true) increase the cooldown
3) And liquidate(alice) also calls _decreaseUserShare(alice,...) with useCooldown = true in the same block, the cooldown will not be expired, causing the call to revert
Impact
A malicious user can keep its position unhealthy by preventing liquidations
Proof of Concept
function test_auditPreventLiquidationByAddingCollateral() public {
// Bob deposits collateral so alice can be liquidated
vm.startPrank(bob);
collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(bob), weth.balanceOf(bob), 0, block.timestamp, false );
vm.stopPrank();
// Deposit and borrow from Alice's account to create basis for liquidation
_depositCollateralAndBorrowMax(alice);
// Cause a drastic price drop to allow for liquidation
_crashCollateralPrice();
vm.expectRevert("Must wait for the cooldown to expire");
collateralAndLiquidity.liquidateUser(alice);
// Warp past the lockout period, so liquidation is now allowed
vm.warp(block.timestamp + 1 hours); // warp to just after the lockout period
//alice add few wai of liquidity, which do not recover her health factor
deal(address(wbtc), alice, 101);
deal(address(weth), alice, 101);
vm.prank(alice);
collateralAndLiquidity.depositCollateralAndIncreaseShare(101, 101, 0, block.timestamp, false );
vm.expectRevert("Must wait for the cooldown to expire");
collateralAndLiquidity.liquidateUser(alice);
}
Tools Used
Manual review
Recommended Mitigation Steps
When the user position is unhealthy, user should be forced to at least add collateral up to a healthy state
Lines of code
https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L107
Vulnerability details
Vulnerability details
Alice can front-run a liquidator call
liquidate(alice)
by callingdepositCollateralAndIncreaseShare
with a value > DUST and DoS/prevent its liquidation, as liquidate is limited bycooldownExpiration
in_decreaseUserShare
Why?
1) every time
_increaseUserShare
or_decreaseUserShare
is called withuseCooldown = true
, a cooldown is set toblock.timestamp + modificationCooldown
. This cooldown will prevent any calls to these functions (ifuseCooldown = true
) until the time has passed 2) BecausedepositCollateralAndIncreaseShare
(which calls_depositLiquidityAndIncreaseShare
, itself calling_increaseUserShare
withuseCooldown == true
) increase the cooldown 3) Andliquidate(alice)
also calls_decreaseUserShare(alice,...)
withuseCooldown = true
in the same block, the cooldown will not be expired, causing the call to revertImpact
A malicious user can keep its position unhealthy by preventing liquidations
Proof of Concept
Tools Used
Manual review
Recommended Mitigation Steps
When the user position is unhealthy, user should be forced to at least add collateral up to a healthy state
Assessed type
Context