Stella

The BandAdapterOracle might return an invalid/stale price if the Arbitrum sequencer goes down. The Chainlink oracle that provides real time information on its function should always be checked before consuming any data.

Function BaseStrategy::liquidatePosition defines the condition that must be satisfied in order for a position to be considered liquidatable:

BaseStrategy::LiquidatePosition():L349-356

if (
    pos.status != PositionStatus.ACTIVE ||
    (debtRatioE18 < ONE_E18 &&
        pos.startLiqTimestamp == 0 &&
    	  pos.positionDeadline > block.timestamp)
) {
    revert PositionNotLiquidatable(
        _params.posOwner, _params.posId, pos.status
    );
}

As one can observe, if liquidatePosition is called and it is true that debtRatioE18 < ONE_E18 but at the same time it holds that pos.startLiqTimestamp != 0, the position can get liquidated. How could this happen:

• debtRatioE18 goes over 100% (ONE_E18)
• the position is marked liquidatable, i.e., pos.startLiqTimestamp is set to a value different from 0
• debtRatioE18 goes below ONE_E18 before the position gets liquidated due to the user adding extra collateral or due to the value of borrowed/collateral assets changing.

At this point the position can get liquidated even though it is above water. At the same time, pos.startLiqTimestamp can only be set via BasePositionManager::markLiquidationStatus, a function which is intended mainly for position monitoring and liquidation bots to call. Thus, the marking and unmarking of positions depends heavily on off-chain code, meaning that there is always a chance that a previously unhealthy position, which becomes healthy, might not be marked as such and might get liquidated unfairly.

As a general security practice, we recommend avoiding the dependence on off-chain logic as much as possible. Ideally, even in an extreme scenario in which all monitoring bots are down for a prolonged period of time, the effects on the protocol should not be unlimited.

In case of liquidatePosition an extreme downtime scenario could lead to liquidating a position with arbitrarily low debt ratio if it remains marked. This could be avoided by checking inside the function whether the conditions for unmarking the position are satisfied, namely that the debt ratio is below unmarkLiqDebtRatioE18.

---

The Stella team acknowledged the issue. Unfortunately, the use of bots for marking/unmarking positions is unavoidable. Also, the team would like to prevent the case where prices fluctuate around the 100% debt ratio, which can cause bots to mark and unmark too many times. That is why there is a different threshold, unmarkLiqDebtRatioE18, for the marking and unmarking. Our final suggestion was to extend the liquidation condition by not allowing the liquidation of positions whose debt ratio is below unmarkLiqDebtRatioE18.

The time-based liquidation discount mechanism offers a 1% discount for each minute a position remains liquidatable. So if a liquidatable position has not been liquidated for 20 minutes, it will have a 20% discount (with a cap at 30%, that is 30 minutes).

In contrast to issue L2, this mechanism does not rely on running our own bot, since anyone has incentive to liquidate a position. Still, the discount logic does interpret the fact that liquidation did not happen within 20 minutes as “lack of incentive”, requiring to lower the price. As a consequence, if the sequencer stays offline for 30 minutes (which is not that unlikely to happen), then when it resumes operation all positions marked as liquidatable before the downtime will have a 30% discount.

If this risk is not acceptable, it could be avoided by requiring liquidators to “enable” the discount at regular intervals, before being allowed to use it. For instance they could be required to call a function every 5 or 10 minutes to enable the discount for the corresponding period, and then wait a bit before using this discount. This essentially proves that the system is live, and that any lack of liquidation is due to insufficient incentives and not due to technical reasons.

The dev and exec multisig wallets have significant power over the protocol as they can set all its different parameters, including access controllers, whitelisted borrowers (as mentioned in issue L1), the RiskFramework contract, the rewards vault, the oracle sources and many more. Nevertheless, the current codebase of the protocol does not provide any direct way to the owners of these wallets, even in a case of wallet compromise, to drain the protocol or steal user funds. The only exceptions to this are

• What is described in issue L1, and
• That the exec multisig is able transfer the whole balance of a rewards vault to the treasury, which is also controlled (set) by the exec multisig.

Although some level of trust to these wallets is necessary, limiting their power as much as possible is preferable.

In function LendingProxy::borrow it is checked that the msg.sender is whitelisted by querying the whitelistedBorrowers mapping, which can be only set by the exec role (multisig). However, it could also be required as an extra security measure that the msg.sender is a strategy registered in the StrategyRegistry contract, i.e., it has been created by one of the whitelisted strategy factories. This would make it much more difficult to add a non-legitimate strategy as a borrower even if the exec multisig got compromised.

The function LendingProxy::shareProfit does not check if the toTreasury and toRewardVault amounts used in the ERC20 safeTransferFrom calls are greater than 0.

In function ChainlinkAdapterOracle::getUSDPriceE36 it is assumed that the number of decimals of the latestRoundData answer is 8 and 18 when the reference asset is USD and ETH respectively. Ideally one should use the decimals value returned by the latestRoundData function as the aforementioned assumption might not hold in the future.

Function ChainlinkAdapterOracle::getUSDPriceE36 could implement an extra check that ensures that the rate returned by the aggregator’s latestRoundData is greater than 0, as we expect that to hold for all asset prices. It could also be checked that the updatedAt value is not completely erroneous by requiring it be less or equal to the block.timestamp.

The function BasePositionViewer::calcProfitInfo can return early, i.e., without calculating the yield and lender profits, which would be equal to 0, when the inputShareE18 is equal to 100%.

The minDesiredHealthFactorE18 check in function getLiquidationDiscountMultiplierE18 of the BasePositionViewer contract should be moved to the start of the function to reduce the consumed gas in cases where the execution reverts due to this check.

The code can be compiled with Solidity versions ^0.8.19. According to the foundry.toml file of the codebase, version 0.8.19 is currently used which has some known bugs, which we do not believe affect the correctness of the contracts.