V3Vault is Vulnerable to Inflation Due to Donation Attacks

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L877

Vulnerability details

Summary

The contract does not have an explicit check or enforcement for a minimum total supply threshold. If the globalLendLimit and dailyLendIncreaseLimitMin are set to low values, an attacker can potentially exploit the contract when the total supply is low enough, leading to a "donation attack".

Proof of Concept

Scenario

1. The contract is deployed with a low globalLendLimit and dailyLendIncreaseLimitMin.
2. During the initial stages of the contract when the total supply is low, an attacker can deposit a large amount of assets (within the limits) and receive a significant portion of the total shares.
3. The attacker can then strategically withdraw or redeem their shares, causing a significant reduction in the total supply.
4. With a very low total supply, the attacker can manipulate the exchange rates between assets and shares by making small deposits or withdrawals, potentially benefiting from the distorted exchange rates.

Impact

Subsequent user deposits can be effectively stolen or significantly diminished due to the inflated exchange rates caused.

Tools Used

Manual

Recommended Mitigation Steps

Introduce an explicit minimum total supply threshold in the contract. This threshold should be set to a reasonable value based on the expected usage and risk profile of the lending protocol.

Add a new constant for the minimum total supply threshold:

uint256 public constant MIN_TOTAL_SUPPLY = 1_000 * 10 ** decimals(); // Example: 1,000 tokens

Modify the _deposit function to check the minimum total supply threshold before minting new shares:

function _deposit(address receiver, uint256 amount, bool isShare, bytes memory permitData)
    internal
    returns (uint256 assets, uint256 shares)
{
    // ...
    shares = _convertToShares(assets, newLendExchangeRateX96, Math.Rounding.Down);

    // Check if the new total supply after minting would be below the minimum threshold
    uint256 newTotalSupply = totalSupply() + shares;
    if (newTotalSupply < MIN_TOTAL_SUPPLY) {
        revert TotalSupplyBelowMinimum();
    }

    _mint(receiver, shares);
    // ...
}

Modify the _withdraw function to check the minimum total supply threshold before burning shares:

function _withdraw(address receiver, address owner, uint256 amount, bool isShare)
    internal
    returns (uint256 assets, uint256 shares)
{
    // ...
    shares = _convertToShares(amount, newLendExchangeRateX96, Math.Rounding.Up);

    // Check if the new total supply after burning would be below the minimum threshold
    uint256 newTotalSupply = totalSupply() - shares;
    if (newTotalSupply < MIN_TOTAL_SUPPLY) {
        revert TotalSupplyBelowMinimum();
    }

    _burn(owner, shares);
    // ...
}

Add a new error event for the TotalSupplyBelowMinimum case:

error TotalSupplyBelowMinimum();

Assessed type

ERC4626

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[c4-pre-sort]

0xEVom marked the issue as primary issue

[0xEVom]

Also see proposed mitigations under #212 and #116

[c4-sponsor]

kalinbas (sponsor) disputed

[kalinbas]

I can not see how this is possible. The shares at the beginning are proportional to to assets deposited.. So if the asset is USDC and 1000000000 are deposited, 1000000000 shares are minted. The exchange rate at the beginning is 1 (2**96)

exchange rates can not be manipulated, they are stored in the contract and are only changed when there is interest added

[jhsagd76]

I have reviewed all the duplicates, and no one has really provided a path for an inflation attack. Direct donations cannot manipulate newLendExchangeRateX96 unless there is a way to manipulate the interest. I have not seen such an attack vector. if there is one, please, warden, supplement it.

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Insufficient proof

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Invalid