Oracle price updates can be front run

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L156-L202

Vulnerability details

Impact

An attacker can repeatedly exploit the protocol and profit by front running oracle price updates. This becomes increasingly financially beneficial as the attacker’s profits compound—incentivizing rapid minting and redeeming of DYAD.

Proof of Concept

When redeeming DYAD for collateral through VaultManagerV2::redeemDyad() the amount of assets a user receives for burning a certain amount of DYAD is determined by the asset price—which is retrieved using a Chainlink oracle:

function redeemDyad(
    ...
)
  external 
    isDNftOwner(id)
  returns (uint) {
      ...
                  / _vault.assetPrice() 
    ...
}

Here is _vault.assetPrice():

function assetPrice() 
  public 
  view 
  returns (uint) {
    (
      ,
      int256 answer,
      , 
      uint256 updatedAt, 
    ) = oracle.latestRoundData();
    if (block.timestamp > updatedAt + STALE_DATA_TIMEOUT) revert StaleData();
    return answer.toUint256();
}

The problem arises due to the fact that an attacker can scan the mempool for oracle updates and front run them. Here is how this can be used by an attacker to exploit the protocol:

1. Scan the mempool for oracle updates.
2. Identify an upcoming increase in answer.
3. Mint DYAD at the current lower price before answer is updated by calling VaultManagerV2::mintDyad().
4. Redeem DYAD for a higher collateral value directly after answer is updated by calling VaultManagerV2::redeemDyad().

Test

The following test demonstrates an attacker increasing the value of their assets by front running an oracle update. Steps to run:

1. Create a file in 2024-04-dyad/test and name it VaultManagerV2.t.sol
2. Pass your RPC URL to createSelectFork()
3. Run forge t --mp test/VaultManagerV2.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.17;

import { Test } from "forge-std/Test.sol";
import { ERC20 } from "@solmate/src/tokens/ERC20.sol";
import { VaultManagerV2 } from "../src/core/VaultManagerV2.sol";
import { DNft } from "../src/core/DNft.sol";
import { Dyad } from "../src/core/Dyad.sol";
import { Licenser } from "../src/core/Licenser.sol";
import { IVaultManager } from "../src/interfaces/IVaultManager.sol";
import { Vault } from "../src/core/Vault.sol";
import { IAggregatorV3 } from "../src/interfaces/IAggregatorV3.sol";

// mock oracle used for PoC purposes
contract MockAggregatorV3 is IAggregatorV3 {
    function decimals() external pure returns (uint8) {return 8;}
    function description() external pure returns (string memory) {return "";}
    function version() external pure returns (uint256) {return 0;}
    function getRoundData(uint80 /*_roundId*/) external pure returns (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) {
        return (0, 0, 0, 0, 0);
    }
    function latestRoundData() external view returns (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) {
        if (block.number < 30000000) {
            return (0, 100000000000, 0, block.timestamp, 0);
        } else {
            return (0, 200000000000, 0, block.timestamp, 0);
        }
    }
}

contract VaultManagerV2Test is Test {
    VaultManagerV2 vaultManagerV2;
    Vault wstETHVault;
    ERC20 wstETH;
    DNft dNft;
    Licenser licenser;
    MockAggregatorV3 mockAggregatorV3;
    Dyad dyad;

    function setUp() public {
        vm.createSelectFork("");
        licenser = new Licenser();
        dNft = new DNft();
        dyad = new Dyad(licenser);
        vaultManagerV2 = new VaultManagerV2(DNft(dNft), Dyad(dyad), Licenser(licenser));
        wstETH = ERC20(payable(0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0));
        mockAggregatorV3 = new MockAggregatorV3();
        wstETHVault = new Vault(IVaultManager(vaultManagerV2), ERC20(wstETH), IAggregatorV3(mockAggregatorV3));
        licenser.add(address(wstETHVault));
        licenser.add(address(vaultManagerV2));
    }

    function test_OracleFrontRun() public {
        // deal the attacker wstETH and mint them a dNFT
        address attacker = makeAddr("attacker");
        uint256 id = dNft.mintNft(attacker);
        deal(address(wstETH), attacker, 10e18);

        // the attacker deposit their wstETHVault in preparation of minting DYAD
        vm.startPrank(attacker);
        wstETH.approve(address(vaultManagerV2), 10e18);
        vaultManagerV2.add(id, address(wstETHVault));
        vaultManagerV2.deposit(id, address(wstETHVault), 10e18);
        vm.stopPrank();

        uint256 attackerTotalBef = vaultManagerV2.getTotalUsdValue(id);

        // attacker scans the mempool and identifies an asset price increase
        uint256 amount = vaultManagerV2.getTotalUsdValue(id) / vaultManagerV2.MIN_COLLATERIZATION_RATIO();
        vm.prank(attacker);
        // front run
        vaultManagerV2.mintDyad(id, amount, attacker);

        // price updates occurs
        vm.roll(30000000);

        uint256 attackerTotalAft = vaultManagerV2.getTotalUsdValue(id);

        // the attackers USD holdings have increased
        assertGt(attackerTotalAft, attackerTotalBef);
    }
}

Tools Used

Manual review & Foundry.

Recommended Mitigation Steps

Consider disabling users from redeeming DYAD immediately after minting it. VaultManagerV2.sol#L156-L202:

+       uint public constant MIN_MINT_DELAY = 90 minutes;
+       mapping (uint => mapping (address => uint)) idToLastMint;

        function mintDyad(
            ...
        )
          external
            isDNftOwner(id)
        {
+           idToLastMint[id][msg.sender] = block.timestamp;
            ...
        }
        ...
        function redeemDyad(
            ...
        )
          external
            isDNftOwner(id)
          returns (uint) {
+               require(block.timestamp >= idToLastMint[id][msg.sender] + MIN_MINT_DELAY, "Deposit too soon");
                ...
        }

Assessed type

MEV

[JustDravee]

Given the effort in convincing, I'm not sure anymore if this is valid or not (would've said no at first, but it's indeed convincing). Seems like an inherent risk everywhere but an actual risk-free profit is never good. Up to the sponsor and the judge to decide

[c4-pre-sort]

JustDravee marked the issue as sufficient quality report

[c4-pre-sort]

JustDravee marked the issue as primary issue

[shafu0x]

yeah front-running an oracle is a general problem. I don't see how to fix that tbh

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid

[c4-judge]

koolexcrypto marked the issue as nullified

[0x175]

Hi @koolexcrypto,

I believe this is a valid finding. I would greatly appreciate any input as to why you believe the issue is invalid.

This is not a general issue applicable to any protocol that integrates with Chainlink. It is a nuanced issue specific to protocols that allow instant redeeming/burning after minting—typically stablecoin protocols. Therefore DYAD is particularly vulnerable.

Leveraging the limitations of an oracle for a risk free profit is never good as JustDravee mentioned. However this issue is not only about the benefit the attacker gets, there is also a downside for legitimate users. As demonstrated in my PoC, when an attacker performs this exploit they generate more value than they put in. This comes from the vault in which they performed the exploit on e.g. the wstETH vault. In this case, users who deposit wstETH into the protocol are compromised as the liquidity within the vault is being exponentially diminished each time an attacker performs this exploit. In severe cases this will lead to users being unable to withdraw/redeem 100% of their DYAD as there will be insufficient liquidity in the vault (particularly vaults with low liquidity).

If the protocol chooses to integrate with an oracle, it is up to them to protect users and mitigate against any issues that arise due to the limitations of the oracle e.g. transparent updates. Especially issues that an attacker can leverage for a risk-free profit at the expense of other users' deposited collateral.

shafu0x’s dispute states that he does not know how to fix this, however the mitigation that I recommended solves the issue. This is a common practice amongst similar protocols that mitigate the same issue.

[InfectedIsm]

Hi @koolexcrypto,

I believe this is a valid finding. I would greatly appreciate any input as to why you believe the issue is invalid.

This is not a general issue applicable to any protocol that integrates with Chainlink. It is a nuanced issue specific to protocols that allow instant redeeming/burning after minting—typically stablecoin protocols. Therefore DYAD is particularly vulnerable.

Leveraging the limitations of an oracle for a risk free profit is never good as JustDravee mentioned. However this issue is not only about the benefit the attacker gets, there is also a downside for legitimate users. As demonstrated in my PoC, when an attacker performs this exploit they generate more value than they put in. This comes from the vault in which they performed the exploit on e.g. the wstETH vault. In this case, users who deposit wstETH into the protocol are compromised as the liquidity within the vault is being exponentially diminished each time an attacker performs this exploit. In severe cases this will lead to users being unable to withdraw/redeem 100% of their DYAD as there will be insufficient liquidity in the vault (particularly vaults with low liquidity).

If the protocol chooses to integrate with an oracle, it is up to them to protect users and mitigate against any issues that arise due to the limitations of the oracle e.g. transparent updates. Especially issues that an attacker can leverage for a risk-free profit at the expense of other users' deposited collateral.

shafu0x’s dispute states that he does not know how to fix this, however the mitigation that I recommended solves the issue. This is a common practice amongst similar protocols that mitigate the same issue.

The protection is already implemented, there's a flashloan protection preventing a user from depositing and withdrawing in the same block: idToBlockOfLastDeposit The VaultManagerV2::redeem function calls VaultManagerV2::withdraw, so its not possible for a user to act as described. Sure, the VaultManagerV2::mint function do not set idToBlockOfLastDeposit, but this attack forcibly require the user to deposit first, before minting. And the only way for this scenario to work, is to deposit in the N-1 block before the redeem, which totally invalidate the idea of price update front-run.

[0x175]

Hi InfectedIsm,

I appreciate your input, however your argument is incorrect. You stated:

"The protection is already implemented, there's a flashloan protection preventing a user from depositing and withdrawing in the same block: idToBlockOfLastDeposit"

This exploit does not require a flash loan nor does it need to be executed within one block. Therefore this line of code does not prevent against this attack as you argued. Instead, it is common practice for protocols similar to DYAD to mitigate this vulnerability by either:

a. Enforce a delay between minting and redeeming (my recommendation); or b. Add a fee to minting and/or redeeming.

[InfectedIsm]

Hey 0x175, thanks for taking time to develop your idea, but I'm still non really convinced this is a vulnerability. Sure it does not require a flashloan, but if the operation isn't executed within one single block, then the arbitrage cannot be predicted right? There is no insurance that the price update in next block will make the operation profitable. If the user need :

1. to deposit its asset in block N
2. then scan the mempool and wait until an update at a block N+M, where the operation profitable

Then it is trading/arbitrage; adding a time delay between deposit and withdraw will not prevent such situation, only delay the arbitrage operation. A fee would probably help preventing such operation, but is trading/arbitrage really an exploit?

I'll let judge decide, wanted to share my understanding of the situation here, I might miss something though

[0x175]

Hi InfectedIsm,

Once again thank you for your input.

The reason why adding a time delay between minting and redeeming prevents the attack is because immediately after the attacker has performed the exploit they are in a position of profit. However, with the added time delay there is no certainty as to what the price will be after the time delay is up—creating a boundary of uncertainty as to whether the attack will be profitable or not. Effectively adding risk to the attack, thus mitigating it. Whereas in the current state of the code, the attacker stands to gain a risk-free profit as they can leverage the fact there is no time delay and almost instantly redeem their DYAD. The attacker does not need to redeem their DYAD within the same block. They can wait until the next block, at which point they will still be in a position of profit.

[Bauchibred]

I'm more inclined to @0x175's position in this discussion, originally submitted the same bug idea as QA-12 in #725.

@InfectedIsm I think you're mistaking the bug flow, for "deposit first" and then "wait till it's profitable", that's not what's done in this case though, here it's a front/back run of the price update from Chainlink, as such we can indeed confirm that it's always going to be profitable, keep in mind that since the oracle in use is from Chainlink, the price is not real time, after any price update from the feeds, the oracles have to either wait for the price deviational threshold to be triggered or the feed's heartbeat to pass before updating the price on-chain again.

So the only case where you can claim the profitability of the flow to be unsure, is if you expect the asset to drop below the deviational threshold within 12 seconds of the first update(since no chainlink feed's heartbeat is as low as 12 seconds), however this still does not solve this issue or make the profitability questionable, cause it's rooted around front/back running the first price update, by the time the second price update gets executed on-chain the attacker has already sold of the minted DYAD since no real waiting duration is placed.

I believe we've all provided enough info and should leave the judge to re-access the final severity.

[koolexcrypto]

Hi @0x175

Thank you for your feedback.

While I agree in general on the issue. I see it not applicable on this protocol specifically. This is because regardless what the price is, as a redeemer, you will never get more that what you deposited as a collateral in the first place. In other words, assume you deposited 10 WETH. No matter how many times you repeat this action, you won't get more that 10 WETH in the end. The only diff is, you get them in different amounts instead.

[c4-judge]

koolexcrypto marked the issue as not nullified

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid