Shambolic Purple Pheasant Liquidity Vulnerability And Mitigation

by Elias Adebayo 65 views

Hey guys, let's dive into a critical security vulnerability found in the Shambolic Purple Pheasant project. This issue revolves around inconsistent liquidity validation after a user exits the market, which can lead to unnecessary liquidity checks and potential gas griefing. This is a medium-severity issue that could impact user experience and protocol efficiency. So, let's break it down and see what's going on.

Summary

The inconsistent liquidity validation in the Operator.sol contract can cause unnecessary liquidity checks and unexpected reverts or excessive gas usage for users who have already exited a market. This happens because the _beforeRedeem() function still performs _getHypotheticalAccountLiquidity() even when a user is no longer a market member. This creates unexpected behavior and opens a gas griefing vector, as the liquidity check is triggered unnecessarily.

Root Cause Analysis

The issue lies within the _beforeRedeem() function in Operator.sol. The intention is to skip the liquidity check if the redeemer isn't a market member:

if (!markets[mToken].accountMembership[redeemer]) return;

However, even after calling exitMarket(), this check doesn't always prevent _getHypotheticalAccountLiquidity() from running. This leads to a few key problems:

  1. A liquidity check is performed regardless of membership status.
  2. There is a gas spike due to the full traversal of accountAssets.
  3. Potential for griefing through a deliberately bloated account asset list.

Let's look at the vulnerable code snippet:

function _beforeRedeem(address mToken, address redeemer, uint256 redeemTokens) private view {
    require(!_paused[mToken][OperationType.Redeem], Operator_Paused());
    require(markets[mToken].isListed, Operator_MarketNotListed());

    /* If the redeemer is not 'in' the market, then we can bypass the liquidity check */
    if (!markets[mToken].accountMembership[redeemer]) return;

    // liquidity check
    (, uint256 shortfall) = _getHypotheticalAccountLiquidity(redeemer, mToken, redeemTokens, 0);
    require(shortfall == 0, Operator_InsufficientLiquidity());
}

The problem here is that the check !markets[mToken].accountMembership[redeemer] doesn't accurately reflect whether a user should be subject to a liquidity check after exiting the market. Even if a user has exited, the system might still perform the liquidity check, leading to unexpected gas costs and potential reverts.

Deep Dive into the Impact

The impact of this vulnerability isn't just a minor inconvenience; it can seriously affect users and the protocol's overall health. Let's break down the potential consequences:

  • Failed Redeem Calls: Users might experience failed redeem() calls due to stale or misaligned liquidity validation logic. Imagine a user trying to withdraw their funds after exiting a market, only to find their transaction failing due to this glitch. This erodes trust and can frustrate users.

  • Gas Griefing Vector: The protocol becomes susceptible to gas griefing. exitMarket() doesn't fully clear the path for gas-efficient redeeming. A malicious user could exploit this by intentionally bloating their account asset list, consuming excessive gas during liquidity checks. This can degrade the user experience for everyone and even impact L2s with tighter block gas limits.

  • Unexpected Gas Costs: Even without malicious intent, users might face unexpectedly high gas costs when trying to redeem their collateral after exiting a market. This is because the system is still performing a liquidity check that shouldn't be necessary, wasting gas and potentially making the transaction too expensive to execute.

This vulnerability highlights the importance of ensuring that liquidity checks are performed only when necessary and that exiting a market truly clears the user from any subsequent liquidity-related validations. Failing to do so can lead to a cascade of negative effects, from user frustration to potential exploitation.

Attack Path

To understand how this vulnerability can be exploited, let's walk through the attack path step by step:

  1. User Deposits and Borrows: A user deposits collateral in the WETH market and borrows assets.
  2. Repays and Exits: The user repays their borrow and then calls exitMarket(WETH).
  3. Redeems Remaining Collateral: The user attempts to redeem() their remaining collateral.
  4. Unnecessary Liquidity Check: Despite not being in the market, the liquidity check is still executed.
  5. Call Fails or Consumes High Gas: The call either fails due to shortfall != 0 or consumes an unexpectedly large amount of gas.

This attack path demonstrates how a user, after legitimately exiting a market, can still be subject to a liquidity check that shouldn't apply to them, leading to either a failed transaction or a significant waste of gas.

Preconditions for the Attack

To successfully execute this attack, certain conditions need to be met:

Internal Pre-Conditions

  1. Market Listing: The owner must list and support a market, such as WETH.
  2. Market Entry: The user must enter the market using enterMarkets([WETH]).
  3. Market Exit: The user must exit the market via exitMarket(WETH).
  4. Redemption Attempt: The user calls redeem() on their remaining collateral.

External Pre-Conditions

  1. Valid Oracle Price: The oracle must return a valid price for the mToken.
  2. Non-Zero Exchange Rate: The exchange rate must be non-zero.

These preconditions set the stage for the vulnerability to be exploited. If these conditions are met, the attack path can be followed, leading to the negative outcomes discussed earlier.

Impact

  • Users may experience failed redeem calls because of misaligned liquidity validation logic.
  • The protocol becomes vulnerable to griefing, as exitMarket() doesn't fully clear the path for gas-efficient redeeming.
  • Malicious users with many assets can intentionally consume excessive gas, degrading the protocol's UX or impacting L2s with tighter block gas limits.

Proof of Concept (PoC)

To demonstrate this vulnerability, the following code can be added to Migrator.Integration.t.sol:

import {mErc20Immutable} from "src/mToken/mErc20Immutable.sol";
import {mErc20Host} from "src/mToken/host/mErc20Host.sol";
import {mErc20} from "src/mToken/mErc20.sol";
import {IPauser} from "src/interfaces/IPauser.sol";
import {Pauser} from "src/pauser/Pauser.sol";
...

mErc20Host public mWethHost;
mErc20Immutable public mWeth;
mErc20 public mErc20_;
Pauser public pauser;
...

mErc20Host implementation = new mErc20Host();
bytes memory initData = abi.encodeWithSelector(
    mErc20Host.initialize.selector,
    address(WETH),
    address(operator),
    address(interestModel),
    1e18,
    "Market WETH",
    "mWeth",
    18,
    payable(address(this)),
    address(zkVerifier),
    address(roles)
);
pauser = new Pauser(address(roles), address(operator), address(this));

...

function test_redeemWithoutLiquidityCheck() public {
    address user = address(this);
    address[] memory toks = new address[](1);
    toks[0] = address(WETH); // WETH is supported market
           
    // Simulate account enters WETH market and deposits tokens
    vm.startPrank(user);
    // Assume WETH listed and supported
    oracleOperator.setUnderlyingPrice(5e18);
    operator.supportMarket(address(WETH));
    operator.setOutflowVolumeTimeWindow(1);
    operator.setOutflowTimeLimitInUSD(1e50);
    operator.setLiquidationIncentive(WETH, 5e18);
    operator.setCollateralFactor(WETH, 0.2 ether);
    operator.setPriceOracle(address(oracleOperator));

    operator.enterMarkets(toks);
    
    // Simulate borrow + repay until allowed to exit market
    // Then exit market
    operator.exitMarket(address(WETH));

    // User is no longer member of WETH market
    // Now attempt to redeem final collateral
    // This call will skip liquidity check
    mWeth.approve(address(WETH), 2 ether);
    mErc20_.redeem(2 ether); // no liquidity check due to !accountMembership
    vm.stopPrank();
}

Running this test will result in a revert, demonstrating the issue:

[FAIL: EvmError: Revert] test_redeemWithoutLiquidityCheck() (gas: 1024184834)

Walking Through the PoC Code

Let's break down the Proof of Concept (PoC) code to understand how it simulates the vulnerability:

  1. Import Statements: The code starts by importing necessary contracts and interfaces, including mErc20Immutable, mErc20Host, mErc20, IPauser, and Pauser. These imports allow the test to interact with the protocol's core components.

  2. Contract Declarations: Several public contract variables are declared, such as mWethHost, mWeth, mErc20_, and pauser. These variables will hold instances of the contracts used in the test.

  3. Initialization: Inside the test setup, the code deploys instances of mErc20Host and Pauser. The mErc20Host is initialized with parameters like the WETH address, operator address, and interest model address. The Pauser contract is initialized with roles and operator addresses.

  4. test_redeemWithoutLiquidityCheck() Function: This function is the core of the PoC. It simulates a user entering and exiting the WETH market and then attempting to redeem their collateral.

    • User Simulation: The vm.startPrank(user) call simulates actions being performed by a specific user.

    • Market Setup: The code sets up the WETH market by setting the underlying price, supporting the market, setting outflow volume limits, setting liquidation incentives, setting the collateral factor, and setting the price oracle.

    • Entering the Market: The user enters the market using operator.enterMarkets(toks), where toks is an array containing the WETH address.

    • Exiting the Market: The user exits the market using operator.exitMarket(address(WETH)). This is a crucial step, as it should theoretically remove the user from the market's liquidity considerations.

    • Attempting to Redeem: The user attempts to redeem their collateral using mErc20_.redeem(2 ether). This is where the vulnerability is triggered. Despite exiting the market, the liquidity check is still performed due to the flawed logic in _beforeRedeem().

    • Expected Revert: The test is expected to revert because the liquidity check fails, demonstrating the vulnerability.

  5. Result: The [FAIL: EvmError: Revert] output confirms that the test failed as expected, proving that the unnecessary liquidity check is indeed performed, leading to a revert.

This PoC effectively demonstrates how a user can be subject to a liquidity check even after exiting a market, leading to failed transactions and potential gas wastage. It underscores the importance of fixing this vulnerability to ensure a smooth and gas-efficient user experience.

Mitigation Steps

To fix this, update _beforeRedeem() to avoid skipping the liquidity check unconditionally based on membership. Instead, apply proper checks using tokensHeld > 0 or handle the edge case correctly:

function _beforeRedeem(address mToken, address redeemer, uint256 redeemTokens) private view {
    require(!_paused[mToken][OperationType.Redeem], Operator_Paused());
    require(markets[mToken].isListed, Operator_MarketNotListed());

    (uint256 tokensHeld,,) = ImToken(mToken).getAccountSnapshot(redeemer);
    if (tokensHeld == 0) return;

    (, uint256 shortfall) = _getHypotheticalAccountLiquidity(redeemer, mToken, redeemTokens, 0);
    require(shortfall == 0, Operator_InsufficientLiquidity());
}

Alternatively, you could always perform the liquidity check regardless of accountMembership. Both approaches ensure that liquidity checks are only performed when necessary, preventing unnecessary gas costs and potential reverts.

Diving Deeper into the Mitigation Strategies

Let's explore the proposed mitigation strategies in more detail to understand their effectiveness and potential trade-offs:

1. Conditional Liquidity Check Based on tokensHeld

This approach involves modifying the _beforeRedeem() function to check if the user holds any tokens in the market before performing the liquidity check. This is achieved by using the getAccountSnapshot() function to retrieve the user's token balance (tokensHeld). If the user holds zero tokens, the liquidity check is skipped; otherwise, it proceeds as usual.

function _beforeRedeem(address mToken, address redeemer, uint256 redeemTokens) private view {
    require(!_paused[mToken][OperationType.Redeem], Operator_Paused());
    require(markets[mToken].isListed, Operator_MarketNotListed());

    (uint256 tokensHeld,,) = ImToken(mToken).getAccountSnapshot(redeemer);
    if (tokensHeld == 0) return;

    (, uint256 shortfall) = _getHypotheticalAccountLiquidity(redeemer, mToken, redeemTokens, 0);
    require(shortfall == 0, Operator_InsufficientLiquidity());
}

Advantages:

  • Gas Efficiency: This method is gas-efficient because it avoids unnecessary liquidity checks for users who have completely exited the market and hold no tokens.
  • Accuracy: It accurately reflects whether a user should be subject to a liquidity check, as it directly checks the token balance.

Potential Considerations:

  • Edge Cases: It's crucial to ensure that getAccountSnapshot() accurately reflects the user's token balance in all scenarios, including edge cases like token transfers or other complex interactions.

2. Always Perform the Liquidity Check

This alternative strategy involves removing the conditional check based on accountMembership and always performing the liquidity check in _beforeRedeem(). While this might seem less efficient at first glance, it can simplify the logic and prevent potential edge cases.

Advantages:

  • Simplicity: This approach simplifies the code by removing the conditional logic, making it easier to reason about and maintain.
  • Consistency: It ensures that the liquidity check is always performed, which can prevent unexpected behavior and potential vulnerabilities related to skipping the check in certain scenarios.

Potential Considerations:

  • Gas Cost: This method might be slightly more gas-intensive, as it performs the liquidity check even for users who have exited the market. However, the gas cost might be negligible compared to the complexity and potential risks of conditional checks.

Choosing the Right Mitigation Strategy

The choice between these two mitigation strategies depends on the specific priorities and trade-offs of the protocol. If gas efficiency is a top priority and the getAccountSnapshot() function is reliable, the conditional check based on tokensHeld is a good option. However, if simplicity and consistency are more important, always performing the liquidity check might be the better choice.

In either case, thorough testing and auditing are crucial to ensure that the chosen mitigation strategy effectively addresses the vulnerability without introducing new issues. It's also essential to consider the long-term maintainability and scalability of the solution.

Conclusion

In summary, the inconsistent liquidity validation in the Shambolic Purple Pheasant project poses a significant risk. By addressing this issue with the suggested mitigations, the protocol can ensure a smoother, more gas-efficient experience for its users and prevent potential gas griefing attacks. Always remember, security is a continuous process, and addressing vulnerabilities like this is crucial for building a robust and trustworthy DeFi ecosystem. Keep learning, keep building, and stay secure, guys!