hats-finance / VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f

LP token lending protocol
MIT License
2 stars 1 forks source link

LendingPool.liquidationCall() Blacklisted liquidator can bypass the `_checkWhitelistBlacklist` check for liquidation by passing the `receiveAToken` as false. #39

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @aktech297 Submission hash (on-chain): 0x72efe4c83407a5113a95a8c1376e62b4225561427d9a2777fddd53bdf6be3d50 Severity: high severity

Description: Summary : For `liquidationCall, blacklisted liquidators are not allowed. This was checked with sponsor and the same can be seen in the natspec comments. // note: liquidators should not be restricted to whitelisted users and ban blacklisted users.

But, this check can be bypassed by passing the bool receiveAToken as false.

Issue details:

wehn we look at the call flow when receiveAToken is set true

liquidationCall -> liquidationCall -> transferOnLiquidation

for both borrower as well as for liquidator, the blacklisted check is done inside the

transferOnLiquidation call.

transferOnLiquidation -> _transfer.-> finalizeTransfer

The blacklisted chedk is inside the finalizeTransfer call..

But, when receiveAToken is set as false, following logic will be executed inside the liquidationCall

https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/blob/fb396a3fa412e643de7d8a1fd8a0268ab3a2f993/packages/contracts/contracts/protocol/lendingpool/LendingPoolCollateralManager.sol#L250-L268

There are no check to validate for the blacklisted liquidator.

Note : since the validate flag is set to false inside the function call, transferOnLiquidation

function transferOnLiquidation(
    address from,
    address to,
    uint256 value
) external override onlyLendingPool {
    // Being a normal transfer, the Transfer() and BalanceTransfer() are emitted
    // so no need to emit a specific event here
    _transfer(from, to, value, false); ------------------@@@ audit - last argument

    emit Transfer(from, to, value);
}

function _transfer(
    address from,
    address to,
    uint256 amount,
    bool validate
) internal {
    address underlyingAsset = _underlyingAsset;
    ILendingPool pool = _pool;

    uint256 index = pool.getReserveNormalizedIncome(
        underlyingAsset,
        _tranche
    );

    uint256 fromBalanceBefore = super.balanceOf(from).rayMul(index);
    uint256 toBalanceBefore = super.balanceOf(to).rayMul(index);

    super._transfer(from, to, amount.rayDiv(index));

    if (validate) { --------------------------------------------->>>>> this will not be called.
        pool.finalizeTransfer(
            underlyingAsset,
            _tranche,
            from,
            to,
            amount,
            fromBalanceBefore,
            toBalanceBefore
        );
    }

    emit BalanceTransfer(from, to, amount, index);
}

Impact :

Blacklisted check will not prevent the calling of the liquidation call by the blacklisted user.

Fix :

We suggest to add the blacklist check inside the function call liquidationCall

ksyao2002 commented 1 year ago

Sorry for the miscommunication: liquidation calls can be called by anyone. We do not enforce checks that blacklisted users cannot perform liquidation calls, as the blacklisted user could easily just create another address and use that new address to call liquidation.

aktech297 commented 1 year ago

No problem.. thanks for clarifying