Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small mint cost enables griefing/DoS via mass-minting of tokens to deplete the protocol's Chainlink VRF subscription LINK funds #920

Closed
c4-submissions opened this issue Nov 10, 2023 · 7 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-1464 unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L157-L166
https://github.com/smartcontractkit/chainlink/blob/develop/contracts/src/v0.8/vrf/VRFCoordinatorV2.sol#L563-L565
https://github.com/smartcontractkit/chainlink/blob/develop/contracts/src/v0.8/vrf/VRFCoordinatorV2.sol#L574-L595

Vulnerability details

Impact

If a collection's minting cost is set to a very low value, an attacker can grief/DoS the protocol by minting many tokens to deplete the LINK funds in the protocol's Chainlink VRF subscription. Any collection using the RandomizerVRF contract will not be able to mint NFTs properly, and the protocol will lose money.

Proof of Concept

Suppose a collection's minting cost is set to 1 wei. This is possible because there are no lower bound checks in MinterContract.setCollectionCosts():

    function setCollectionCosts(uint256 _collectionID, uint256 _collectionMintCost, uint256 _collectionEndMintCost, uint256 _rate, uint256 _timePeriod, uint8 _salesOption, address _delAddress) public CollectionAdminRequired(_collectionID, this.setCollectionCosts.selector) {
        require(gencore.retrievewereDataAdded(_collectionID) == true, "Add data");
        collectionPhases[_collectionID].collectionMintCost = _collectionMintCost; // No lower bound check
        collectionPhases[_collectionID].collectionEndMintCost = _collectionEndMintCost;
        collectionPhases[_collectionID].rate = _rate;
        collectionPhases[_collectionID].timePeriod = _timePeriod;
        collectionPhases[_collectionID].salesOption = _salesOption;
        collectionPhases[_collectionID].delAddress = _delAddress;
        setMintingCosts[_collectionID] = true;
    }

In this scenario the royalties collected by the protocol will be very low compared to the fees paid to Chainlink VRF, so the protocol will lose money if NFTs are minted in this collection. This attack could occur if an artist proposes a collection with a very small minting fee, or if an authorized caller of setCollectionCosts() is compromised.

If desired, see the below VRF LINK fee amount calculation from Chainlink's VRFCoordinatorV2 contract:

  function _calculatePaymentAmount(
    uint256 startGas, //gasLeft() at start of call to fulfillRandomWords from the oracle
    uint256 gasAfterPaymentCalculation, //extra gas fee determined by state variable in VRFCoordinatorV2
    uint32 fulfillmentFlatFeeLinkPPM,
    uint256 weiPerUnitGas // tx.gasprice
  ) internal view returns (uint96) {
    int256 weiPerUnitLink;
    weiPerUnitLink = _getFeedData();
    if (weiPerUnitLink <= 0) {
      revert InvalidLinkWeiPrice(weiPerUnitLink);
    }
    // Will return non-zero on chains that have this enabled
    uint256 l1CostWei = ChainSpecificUtil._getCurrentTxL1GasFees(msg.data);
    // (1e18 juels/link) ((wei/gas * gas) + l1wei) / (wei/link) = juels
    uint256 paymentNoFee = (1e18 * (weiPerUnitGas * (gasAfterPaymentCalculation + startGas - gasleft()) + l1CostWei)) /
      uint256(weiPerUnitLink); // This calculation is dependent on the amount of gas spent in the protocol's VRF callback, at least 2100 due to a cold SSTORE in the call to NextGenCore.setTokenHash()
    uint256 fee = 1e12 * uint256(fulfillmentFlatFeeLinkPPM);
    if (paymentNoFee > (1e27 - fee)) {
      revert PaymentTooLarge(); // Payment + fee cannot be more than all of the link in existence.
    }
    return uint96(paymentNoFee + fee);
  }

Recommended Mitigation

Don't allow minting cost to be set below a certain value. For example, don't allow the minting cost to be set below a value such that the Chainlink VRF fees cost more than the royalties paid to the protocol from minting. Such a check would cost an insignificant amount of gas, especially since setCollectionCosts() is not a high-traffic function.

Assessed type

DoS

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 10, 2023
c4-submissions added a commit that referenced this issue Nov 10, 2023
@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #1255

@c4-judge
Copy link

c4-judge commented Dec 6, 2023

alex-ppg marked the issue as duplicate of #1464

@c4-judge
Copy link

c4-judge commented Dec 9, 2023

alex-ppg marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Dec 9, 2023
@0xDetermination
Copy link

0xDetermination commented Dec 9, 2023

@alex-ppg Hi, thank you for judging. I believe this submission is not a duplicate of #1464 , because that issue is about VRF callback failure, whereas this issue does not address VRF callback failure at all.

The crux of this issue is that the protocol contract requires a VRF subscription to get random data, which costs LINK tokens for every randomness request. This is meant to cover the gas fees of the VRF provider and also to pay some fees to the service. Therefore, if a NFT collection's minting cost is set to an extremely low value, then a minter can grief the protocol by minting for comparatively low cost in order to deplete the protocol's subscription funds.

@alex-ppg
Copy link

alex-ppg commented Dec 9, 2023

Hey @0xDetermination, thanks for flagging this! My response in #1464 details that a lot of Chainlink-related issues were grouped, you can see it here: #1464 (comment)

Randomizers are configured by the administrators of NextGen and they can remove a collection's randomizer if they deem it is attempting to grief the system. Additionally, the NextGen system is heavily centralized and such a collection can be eliminated. Even if a minimum collection price was established, the native asset of Ethereum is highly volatile rendering this type of restriction difficult if possible to enforce.

@0xDetermination
Copy link

0xDetermination commented Dec 10, 2023

Hi @alex-ppg , Thank you very much for the reply and for the info about the grouping of Chainlink-related issues. I will not dispute the invalidation of issues describing randomizer nuisances.

That being said, the issue here is not about a randomizer nuisance or malicious randomizer. Rather, it is about griefing from a malicious user. If a Chainlink VRF randomizer is used, the protocol MUST subscribe the minting contract to the VRF contract, depositing LINK tokens to pay proper fees to the randomness provider upon every randomness request (will at least cover the cost of gas used by the provider to submit on-chain random data). This is required behavior, and there is no malicious element here. The malicious element is if a collection has an extremely small mint cost, such as 1 wei, and a user spam-mints to deplete the protocol's VRF subscription funds. The cost to the attacker here is basically just the gas required to complete the minting transaction. Furthermore, high minting traffic on that collection can deplete the protocol's LINK balance and cause the protocol to lose funds, and I don't see how this would be a desired behavior.

To address your point about collections with small mint cost being removed, I agree that this is true- but if this issue is not previously known by the protocol, it will be difficult to detect this attack until a significant amount of damage has already been done.

I also agree that due to the fluctuation of ETH's value relative to LINK (in addition to variable network fees), it is impossible to enforce a 'perfect' minimum minting value. However, it is still possible to enforce some relatively small minimum to increase the cost of the attack. I think it's an appropriate 'sanity check' that the minting cost should not be able to be as small as 1 wei.

@alex-ppg
Copy link

Hey @0xDetermination, thanks for contributing with your clarification! Your recommendation of imposing a cost that is not as small as 1 wei would still be insufficient in protecting LINK depletion, and the codebase also supports certain models that are not meant to impose any price (i.e. burn-to-mint, auction models). Regardless of the minimum, those functions would still be susceptible to spam calls and thus to depleting the protocol of LINK funds.

I understand the concern and do not consider it a viable strategy for a protocol to take on the cost of randomness requests. However, this is a principle upon which the NextGen system is built and even though I may personally consider it unrealistic it is a statement that we need to take as "fact" when evaluating the project's submissions.

For example, participating in the NextGen system appears to undergo a manual application process meaning that the NextGen team can control which users utilize their system, what price models they implement, etc. Proper analysis of this data could render the system viable, and this is what we assume as "fact". Based on this, I will continue to consider fund depletion attacks around oracles as invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-1464 unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

5 participants