Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/libraries/logic/LiquidationLogic.sol#L39

Vulnerability details

Impact

Tokens like USDT & USDC implement blacklist and pausable functionality in the contract. Let's take a look at the liquidation process in LiquidationLogic.sol::liquidate:

 function liquidate(
        uint256 vaultId,
        uint256 closeRatio,
        GlobalDataLibrary.GlobalData storage globalData,
        bytes memory settlementData
    ) external returns (IPredyPool.TradeResult memory tradeResult) {

        //REDACTED by erictee

        if (!hasPosition) {
            int256 remainingMargin = vault.margin;

            if (remainingMargin > 0) {
                if (vault.recipient != address(0)) {
                    // Send the remaining margin to the recipient.
                    vault.margin = 0;

                    sentMarginAmount = uint256(remainingMargin);

                    ERC20(pairStatus.quotePool.token).safeTransfer(vault.recipient, sentMarginAmount); //erictee-issue: blacklist/pause can cause revert here.
                }
            } else if (remainingMargin < 0) {
                vault.margin = 0;

                // To prevent the liquidator from unfairly profiting through arbitrage trades in the AMM and passing losses onto the protocol,
                // any losses that cannot be covered by the vault must be compensated by the liquidator
                ERC20(pairStatus.quotePool.token).safeTransferFrom(msg.sender, address(this), uint256(-remainingMargin));//erictee-issue: blacklist/pause can cause revert here.
            }
        }

The contract attempt to call safeTransfer/safeTransferFrom for specific ERC20 tokens. If the tokens used is USDT or USDC, when those tokens are paused or certain addresses are blacklisted, denial of service scenario in liquidation process can occur.

Additionally, a vault owner can set the vault.recipient to an already blacklisted address to prevent his vault from getting liquidated.

This is a high risk security issue as it can create bad debt to the protocol as a result of not being able to liquidate certain position immediately.

Proof of Concept

Add MockERC20WithBlacklist.sol in test/mocks/MockERC20WithBlacklist.sol with the following content:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import {ERC20} from "@solmate/src/tokens/ERC20.sol";

/**
 * @notice Mock of ERC20 contract
 */
contract MockERC20WithBlacklist is ERC20 {
    mapping(address => bool) public isBlackListed;
    bool public paused = false;

    modifier whenNotPaused() {
        require(!paused);
        _;
    }

    modifier whenPaused() {
        require(paused);
        _;
    }

    constructor(string memory _name, string memory _symbol, uint8 _decimals) ERC20(_name, _symbol, _decimals) {}

    function mint(address account, uint256 amount) public {
        _mint(account, amount);
    }

    function pause() public whenNotPaused {
        paused = true;
    }

    function unpause() public whenPaused {
        paused = false;
    }

    function addBlackList(address _evilUser) public {
        isBlackListed[_evilUser] = true;
    }

    function removeBlackList(address _clearedUser) public {
        isBlackListed[_clearedUser] = false;
    }

    function transfer(address _to, uint256 _value) public virtual override whenNotPaused returns (bool) {
        require(!isBlackListed[msg.sender]);
        return super.transfer(_to, _value);
    }

    function transferFrom(address _from, address _to, uint256 _value)
        public
        virtual
        override
        whenNotPaused
        returns (bool)
    {
        require(!isBlackListed[_from]);

        return super.transferFrom(_from, _to, _value);
    }
}

Modify test/pool/Setup.t.sol to use MockERC20WithBlacklist instead of MockERC20:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import {IUniswapV3Factory} from "@uniswap/v3-core/contracts/interfaces/IUniswapV3Factory.sol";
import {IUniswapV3Pool} from "@uniswap/v3-core/contracts/interfaces/IUniswapV3Pool.sol";
import {IUniswapV3PoolActions} from "@uniswap/v3-core/contracts/interfaces/pool/IUniswapV3PoolActions.sol";
import {TransferHelper} from "@uniswap/v3-periphery/contracts/libraries/TransferHelper.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {TickMath} from "@uniswap/v3-core/contracts/libraries/TickMath.sol";
import "../../src/PredyPool.sol";
import "../../src/libraries/InterestRateModel.sol";
import "../mocks/MockERC20.sol";
++  import "../mocks/MockERC20WithBlacklist.sol";
import "../mocks/TestTradeMarket.sol";
import "../../src/settlements/UniswapSettlement.sol";
import "../mocks/DebugSettlement2.sol";
import "../../src/lens/PredyPoolQuoter.sol";
import "../../src/interfaces/IFillerMarket.sol";

contract TestPool is Test {
    PredyPool predyPool;

--  MockERC20 currency0;
--  MockERC20 currency1;
++  MockERC20WithBlacklist currency0;
++  MockERC20WithBlacklist currency1;

    IUniswapV3Pool internal uniswapPool;

    uint128 internal constant RISK_RATIO = 109544511;
    uint128 internal constant BASE_MIN_COLLATERAL_WITH_DEBT = 2000;

    address uniswapFactory;

    PredyPoolQuoter _predyPoolQuoter;

    UniswapSettlement uniswapSettlement;
    DebugSettlement2 debugSettlement;

    function setUp() public virtual {
--      currency0 = new MockERC20("currency0", "currency0", 18);
--      currency1 = new MockERC20("currency1", "currency1", 18);
++      currency0 = new MockERC20WithBlacklist("currency0", "currency0", 18);
++      currency1 = new MockERC20WithBlacklist("currency1", "currency1", 18);
// REDACTED by erictee

Add testLiquidateFailsIfVaultIsDangerWhenPaused test in test/market/perp/ExecLiquidationCall.t.sol with the following content:

function testLiquidateFailsIfVaultIsDangerWhenPaused() public {
    PerpOrderV3 memory order = PerpOrderV3(
        OrderInfo(address(perpMarket), from1, 0, block.timestamp + 100),
        1,
        address(currency1),
        "Sell",
        4 * 1e8,
        101000000,
        0,
        0,
        4,
        false,
        false,
        abi.encode(PerpMarketLib.AuctionParams(Constants.Q96 / 2, Constants.Q96 / 2, 0, 0))
    );

    IFillerMarket.SignedOrder memory signedOrder = _createSignedOrder(order, fromPrivateKey1);

    perpMarket.executeOrderV3(signedOrder, _getUniSettlementDataV3(0));

    _movePrice(true, 6 * 1e16);

    vm.warp(block.timestamp + 30 minutes);

    uint256 beforeMargin = currency1.balanceOf(from1);
    currency1.pause(); //erictee: Token contract is paused before liquidation happens.
    perpMarket.execLiquidationCall(1, 1e18, _getUniSettlementDataV3(Constants.Q96 * 12 / 10));
    uint256 afterMargin = currency1.balanceOf(from1);

    assertGt(afterMargin - beforeMargin, 0);
}

Run the test with command: forge test --mt testLiquidateFailsIfVaultIsDangerWhenPaused -vvv.

Result:

mac@macs-MacBook-Pro 2024-05-predy % forge test --mt testLiquidateFailsIfVaultIsDangerWhenPaused -vv 
[⠑] Compiling...
No files changed, compilation skipped

Running 1 test for test/market/perp/ExecLiquidationCall.t.sol:TestPerpExecLiquidationCall
[FAIL. Reason: revert: TRANSFER_FAILED] testLiquidateFailsIfVaultIsDangerWhenPaused() (gas: 1737941)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 35.38ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/market/perp/ExecLiquidationCall.t.sol:TestPerpExecLiquidationCall
[FAIL. Reason: revert: TRANSFER_FAILED] testLiquidateFailsIfVaultIsDangerWhenPaused() (gas: 1737941)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual Analysis

Recommended Mitigation Steps

Allow the recipient address to claim their remaining margin instead of sending the token to them directly during liquidation process.

Alternatively, restrict the use of tokens with pausable functionality.

Assessed type

DoS

code-423n4 / 2024-05-predy-findings

Denial of service in liquidation process in `LiquidationLogic.sol::liquidate` if the recipient address is blacklisted in certain tokens or the tokens are paused. #165