sherlock-audit / 2023-04-ajna-judging

4 stars 3 forks source link

branch_indigo - Malicious users can manipulate spot LUP to kick borrower's loan, causing borrowers penalized with more debts #74

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

branch_indigo

medium

Malicious users can manipulate spot LUP to kick borrower's loan, causing borrowers penalized with more debts

Summary

Spot Lup(lowest utilization price) can be manipulated and a borrower's account health is evaluated based on spot Lup.

Vulnerability Detail

When a borrower's account health is evaluated, spot Lup price is used. To kick an account, Pool.sol kick() is invoked which calls KickerActions.sol, which under the hood calls internal _kick() where account collateralization is evaluated.

//Pool.sol
    function kick(
        address borrower_,
        uint256 npLimitIndex_
    ) external override nonReentrant {
        PoolState memory poolState = _accruePoolInterest();
        // kick auction
|>        KickResult memory result = KickerActions.kick(
            auctions,
            deposits,
            loans,
            poolState,
            borrower_,
            npLimitIndex_
        );
...
//KickerActions.sol
    function kick(
        AuctionsState storage auctions_,
        DepositsState storage deposits_,
        LoansState    storage loans_,
        PoolState calldata poolState_,
        address borrowerAddress_,
        uint256 limitIndex_
    ) external returns (
        KickResult memory
    ) {
|>        return _kick(
            auctions_,
            deposits_,
            loans_,
            poolState_,
            borrowerAddress_,
            limitIndex_,
            0
        );
    }
//KickerActions.sol
    function _kick(
        AuctionsState storage auctions_,
        DepositsState storage deposits_,
        LoansState    storage loans_,
        PoolState calldata poolState_,
        address borrowerAddress_,
        uint256 limitIndex_,
        uint256 additionalDebt_
    ) internal returns (
        KickResult memory kickResult_
    ) {
...
      Borrower storage borrower = loans_.borrowers[borrowerAddress_];
        kickResult_.debtPreAction       = borrower.t0Debt;
        kickResult_.collateralPreAction = borrower.collateral;
        kickResult_.t0KickedDebt        = kickResult_.debtPreAction ;
        // add amount to remove to pool debt in order to calculate proposed LUP
|>        kickResult_.lup          = Deposits.getLup(deposits_, poolState_.debt + additionalDebt_);
        KickLocalVars memory vars;
        vars.borrowerDebt       = Maths.wmul(kickResult_.t0KickedDebt, poolState_.inflator);
        vars.borrowerCollateral = kickResult_.collateralPreAction;

        // revert if kick on a collateralized borrower
|>        if (_isCollateralized(vars.borrowerDebt, vars.borrowerCollateral, kickResult_.lup, poolState_.poolType)) {
            revert BorrowerOk();
        }
...

As seen from above, before _isCollateralized(), the current states of deposits_ and poolState_.debt is assessed to calculated spot kickResult_.lup which is used as the price for collateralization.

//PoolHelper.sol
    function _isCollateralized(
        uint256 debt_,
        uint256 collateral_,
        uint256 price_,
        uint8 type_
    ) pure returns (bool) {
|>        if (type_ == uint8(PoolType.ERC20)) return Maths.wmul(collateral_, price_) >= debt_;
...

Since states of deposits_ is updated every transaction when there is a change of quote tokens and poolState reflects poolBalances which is updated every transaction when there is a change of debt, this means that kickResult_.lup can be manipulated with a transaction causing a healthy borrower to be kicked.( Note that new borrow debt from drawDebt() is stored in poolBalances and then carried through memory poolState without a delay in _accruePoolInterest() at the beginning of kick().)

For example, a malicious user can take on large loans to lower spot LUP prices to allow some high-leveraged borrowers to be undercollateralized. In the same transaction, the malicious user can kick the borrowers who will be instantly punished with 90-day interest and wait for auctions to start.

//ERC20Pool.sol->drawDebt()
//note: poolBalances updated at end of drawDebt()
...
        if (amountToBorrow_ != 0) {
            // update pool balances state
            poolBalances.t0Debt = poolState.t0Debt;
            // move borrowed amount from pool to sender
            _transferQuoteToken(msg.sender, amountToBorrow_);
        }
...
//KickerActions.sol->_kick()
...
        // when loan is kicked, penalty of three months of interest is added
        vars.t0KickPenalty = Maths.wdiv(Maths.wmul(kickResult_.t0KickedDebt, poolState_.rate), 4 * 1e18);
        vars.kickPenalty   = Maths.wmul(vars.t0KickPenalty, poolState_.inflator);

        kickResult_.t0PoolDebt   = poolState_.t0Debt + vars.t0KickPenalty;
|>        kickResult_.t0KickedDebt += vars.t0KickPenalty;

        // update borrower debt with kicked debt penalty
        ////note: this update the penalty to borrower t0 debt balance.
        borrower.t0Debt = kickResult_.t0KickedDebt;
...

Impact

High-leveraged borrowers can be unfairly kicked due to spot LUP manipulation, penalized and set up for auctions.

This is different from accounts kicked due to normal market activities or accumulated interests. And when the pool liquidity is low, malicious users have better shots to target specific borrowers and manipulate spot LUPs.

Code Snippet

https://github.com/ajna-finance/ajna-core/blob/e3632f6d0b196fb1bf1e59c05fb85daf357f2386/src/libraries/helpers/PoolHelper.sol#L164

https://github.com/ajna-finance/ajna-core/blob/e3632f6d0b196fb1bf1e59c05fb85daf357f2386/src/libraries/external/KickerActions.sol#L385-L392

Tool used

Manual Review

Recommendation

Consider not using calculated kickResult._lupin _isCollateralized(). Evaluate a borrower's collateralization based on a delayed stored LUP instead of spot LUP. The delayed LUP is similar to Uniswap v3's observation, which can be written to storage at the beginning of the next block. timestamp. This mitigates such attacks so that the manipulated spot price cannot be accessed in the same transaction in _isCollateralized().

hrishibhat commented 1 year ago

Lead Watson comment:

looks to be invalid as kicking with manipulated LUP is guarded by kick deposit concept. I.e. it focuses on a surface that is covered by design. One can argue that it’s met only partially, but the bottom line is that’s a known vector that is alleviated by design itself.

Sponsor comment:

Correct, #74 is Invalid