Wild Credit contest - cmichel's results

Handle

cmichel

Vulnerability details

There is no check in UniswapV3Oracle.ethPrice if the return values indicate stale data. This could lead to stale prices according to the Chainlink documentation:

• under current notifications: "if answeredInRound < roundId could indicate stale data."
• under historical price data: "A timestamp with zero value means the round is not complete and should not be used."

It also seems to use the deprecated API (latestAnswer) and a hardcoded decimals of 1e8?

This API is deprecated. Please see API Reference for the latest Price Feed API. Chainlink Docs

Impact

Stale prices that do not reflect the current market price anymore could be used which would influence the liquidation pricing.

Recommendation

Add the recommended checks:

(
    uint80 roundID,
    int256 price,
    ,
    uint256 timeStamp,
    uint80 answeredInRound
) = chainlink.latestRoundData();
require(
    timeStamp != 0,
    “ChainlinkOracle::getLatestAnswer: round is not complete”
);
require(
    answeredInRound >= roundID,
    “ChainlinkOracle::getLatestAnswer: stale data”
);
require(price != 0, "Chainlink Malfunction”);

Handle

cmichel

Vulnerability details

The LendingPair.withdrawUniPosition function allows the user to withdraw their UniswapV3 pool position (NFT) again. As the Uniswap position acts as collateral in the protocol, a health check is performed afterwards.

However, it does not check the current debt of the caller as it does not accrue the debt for both tokens first.

Impact

In the worst case, in low-activity markets, it could happen that debt has not accrued for a long time and the current debt is significantly higher than the current recorded debt in totalDebtAmount. An account with a de-facto negative health ratio if the debt was accrued could still withdraw their collateral NFT instead of having to repay their debt first.

Recommendation

Accrue the debt for both tokens first in LendingPair.withdrawUniPosition.

Handle

cmichel

Vulnerability details

The LendingPair.uniClaimDeposit function allows the user to "collect fees" and mint new supply shares with the collected amounts.

uniClaimDeposit does not accrue tokens

However, the current total supply is not accrued in the function. This means an attacker can:

• mint shares using uniClaimDeposit
• increase the totalSupplyAmount by calling accrue(token0) and accrue(token1) afterwards.
• call withdraw and receive a larger amount of tokens for the newly minted shares due to the increase in totalSupplyAmount from accrue (increasing the supply share price _sharesToSupply).

This would only lead to a small protocol loss if uniClaimDeposit would only collect the fees, however, combined with another flaw, one can steal almost the entire protocol lpRate each time:

uniClaimDeposit allows collecting entire liquidity instead of just fees

This has to do with the way liquidity from a Uniswap V3 position (NFT) is withdrawn:

• when calling positionManager.decreaseLiquidity, the position.liquidity is removed but stored in the position as tokensOwed0/tokensOwed1. It is not transferred to the user
• one needs to call positionManager.collect(params) to actually transfer out these tokens, setting tokensOwed0/1 to 0. (This is correctly done in UniswapV3Helper.removeLiquidity.)

An attacker can perform the following attack:

• Create a Uniswap V3 position.
• Get flashloans for both tokens to provide lots of liquidity for this position.
• Call positionManager.decreaseLiquidity such that the entire liquidity is removed and stored (but not collected yet) in the position's tokensOwed0/1 fields
• Deposit it to WildCredit's lending pair using depositUniPosition
• Call uniClaimDeposit to mint a huge amount of NFT supply shares. This huge amount will capture the protocol's debt accrual in the next steps.
• Call accrue on both tokens to accrue debt and pay the lpRate part of it to suppliers, increasing totalSupplyAmount and thus the value of a supply share.
• With the new debt added to the totalSupplyAmount, the attacker can now withdraw their minted shares again and capture most of the new debt that was accrued, making a profit.

Impact

Combining these two issues, an attacker could steal most of the accrued lpRate in a single atomic transaction. The attacker can repeat this step capturing the supplier interest for each accrual. (The longer the market hasn't been accrued, the bigger the profit per single attack transaction, but in the end, the attacker could perform this attack at every block or when it becomes profitable for the gas costs.)

Providing / removing Uniswap V3 liquidity does not incur fees.

The attacker's profit is the loss of other legitimate suppliers that capture less of the newly accrued debt.

Recommendation

Accrue the debt for both tokens first in LendingPair.uniClaimDeposit.

It might also be a good idea to disallow collecting the "parked" liquidity in a token (that has been removed but not yet collected) by immediately collecting them when the NFT is deposited in depositUniPosition. I.e., call _uniCollectFees in depositUniPosition to withdraw any outstanding tokens&fees. Then mint shares with these token amounts.