Liquidate in execute function, calculates with outdated values
Summary
A user can have a good collateralization ratio and still get liquidated, or have a bad collateral ratio and escape liquidation when calling the execute function
Let's say bob wishes to add collateral into the bank:
The bob makes a call to the Execute function, which then calls the putCollateralfunction using the bob's verified input SPELL. Before the call ends, in the Execute function, checks if the bob's position is liquidatable if (isLiquidatable(positionId)) revert Errors.INSUFFICIENT_COLLATERAL();
The isLiquidatable() function, then checks to confirm if the position is liquidatable or not:
return
getPositionRisk(positionId) >=
banks[positions[positionId].underlyingToken].liqThreshold;
}```
The problem here is, the function `getPositionRisk`() could return the wrong results, here's why:
`getPositionRisk `calculates based on the results of this three functions:
[getPositionValue](https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/BlueBerryBank.sol#L392), [getDebtValue](https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/BlueBerryBank.sol#L422) and [getIsolatedCollateralValue](https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/BlueBerryBank.sol#L433)
function getPositionRisk(
uint256 positionId
) public view returns (uint256 risk) {
uint256 pv = getPositionValue(positionId);
uint256 ov = getDebtValue(positionId);
uint256 cv = getIsolatedCollateralValue(positionId);
if (
(cv == 0 && pv == 0 && ov == 0) || pv >= ov // Closed position or Overcollateralized position
) {
risk = 0;
} else if (cv == 0) {
// Sth bad happened to isolated underlying token
risk = Constants.DENOMINATOR;
} else {
risk = ((ov - pv) * Constants.DENOMINATOR) / cv;
}
}`
Here's the important part from the function getIsolatedCollateralValue :
``` uint256 underlyingAmount;
if (_isSoftVault(pos.underlyingToken)) {
underlyingAmount =
(ICErc20(banks[pos.debtToken].bToken).exchangeRateStored() *
pos.underlyingVaultShare) /
Constants.PRICE_PRECISION;
} else {
underlyingAmount = pos.underlyingVaultShare;
}
icollValue = oracle.getTokenValue(
pos.underlyingToken,
underlyingAmount
);```
the comments already suggested that accrue be called first before calling the ```getIsolatedCollateralValue```() function, this is because, the exchangeRateStored() returns the exchange rate from the last stored accrued interest, the comment in compound CErc20 implementation, writes " * @dev This function does not accrue interest before calculating the exchange rate".
There's also a similar issue with the function `getDebtValue`():
`getDebtValue`() calls `getPositionDebt`() which uses the `_borrowBalanceStored` for computation, this function also uses the last stored data for computation, which will cause a wrong debt return value:
``` function getPositionDebt(
uint256 positionId
) public view returns (uint256 debt) {
Position memory pos = positions[positionId];
Bank memory bank = banks[pos.debtToken];
if (pos.debtShare == 0 || bank.totalShare == 0) {
return 0;
}
debt = (pos.debtShare * _borrowBalanceStored(pos.debtToken)).divCeil(
bank.totalShare
);
}```
These values can be affected by market conditions such as changes in the underlying asset's price, changes in the interest rate, or changes in the overall liquidity of the market, checking if a users collateral is liquidatable should only be done with the most up to data.
## Impact
A borrower can incur loses due to false liquidation
## Code Snippet
https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/BlueBerryBank.sol#L765
https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/BlueBerryBank.sol#L793
## Tool used
Manual Review
## Recommendation
I recommend adding the modifier `poke(token)` to both `putCollateral`() and `takeCollateral`() functions
Duplicate of #10
Tendency
medium
Liquidate in execute function, calculates with outdated values
Summary
A user can have a good collateralization ratio and still get liquidated, or have a bad collateral ratio and escape liquidation when calling the execute function
Vulnerability Detail
The problem here, is in the putCollateral and the takeCollateral functions.
Let's say bob wishes to add collateral into the bank:
The bob makes a call to the Execute function, which then calls the
putCollateral
function using the bob's verified input SPELL. Before the call ends, in the Execute function, checks if the bob's position is liquidatableif (isLiquidatable(positionId)) revert Errors.INSUFFICIENT_COLLATERAL();
The isLiquidatable() function, then checks to confirm if the position is liquidatable or not:function getPositionRisk( uint256 positionId ) public view returns (uint256 risk) { uint256 pv = getPositionValue(positionId); uint256 ov = getDebtValue(positionId); uint256 cv = getIsolatedCollateralValue(positionId);