search

code-423n4 / 2023-10-nextgen-findings

5 stars 3 forks source link

Anyone can break the randomness of token generation and force to consume the LINK balance of the team intended to be used to request randomness with Chainlink. #1255

Closed c4-submissions closed 11 months ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L227-L232 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L71-L75 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L52-L63 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/ERC721.sol#L245-L251 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L65-L68

Vulnerability details

Impact

Because the user can send a lot of request to the VRF Coordinator of Chainlink to generate randomness, anyone can consume all the LINK tokens allowed by the team to pay the requests and also pre-generate the hash token of a token before it's minted. This break the randomness generation of the tokens/NFT guarantee by the protocol.

Proof of Concept

On the NextGenCore contract, the _mintProcessing() internal functions is called each time a token is minted by the protocol with mint(), burnToMint() and airDropTokens().

    function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {
        tokenData[_mintIndex] = _tokenData;
        collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
        tokenIdsToCollectionIds[_mintIndex] = _collectionID;
        _safeMint(_recipient, _mintIndex);
    }

The protocol can use 3 differents types of contracts to generate randomness, here we use RandomizerVRF contract which use Chainlink. We can see the function call calculateTokenHash() on the randomizer contract to generate the token hash. The RandomizerVRF function work like this: 

    function calculateTokenHash(uint256 _collectionID, uint256 _mintIndex, uint256 _saltfun_o) public {
        require(msg.sender == gencore);
        tokenIdToCollection[_mintIndex] = _collectionID;
        requestRandomWords(_mintIndex);
    }

    function requestRandomWords(uint256 tokenid) public {
        require(msg.sender == gencore);
        uint256 requestId = COORDINATOR.requestRandomWords(
            keyHash,
            s_subscriptionId,
            requestConfirmations,
            callbackGasLimit,  
            numWords
        );
        tokenToRequest[tokenid] = requestId;
        requestToToken[requestId] = tokenid;
    }

Like it's well explained in the Chainlink documentations:
https://docs.chain.link/vrf/v2/subscription#how-vrf-processes-your-request

How VRF processes your request After you submit your request, it is processed using the Request & Receive Data cycle. The VRF coordinator processes the request and determines the final charge to your subscription using the following steps: 1) The VRF coordinator emits an event. 2) The VRF service picks up the event and waits for the specified number of block confirmations to respond back to the VRF coordinator with the random values and a proof (requestConfirmations). 3) The VRF coordinator verifies the proof on-chain, then it calls back the consuming contract fulfillRandomWords function.

Chainlink VRFCoordinatorV2 is called with requestRandomWords() and emits a random request. After requestConfirmations blocks, an oracle VRF node replies to the coordinator with a random word, which supplies the random to the requesting contract via fulfillRandomWords() callback. Interesting things is when the call COORDINATOR.requestRandomWords() is executed, an event is emitted and the request is processing. But if the minting process revert after the call to the VRF Coordinator, the request is sended but all the state revert in the contract. A user can maliciously revert the transaction by trying to send the token to a contract that doesn't implement the onERC721Received() method and revert the _safeMint method. Therefore a user can send a big amount of transactions to the VRF Coordinator for the same tokenId without the token being minted. It causes multiples problems.

Each subscription must maintain a minimum balance to fund requests from consuming contracts. This minimum balance requirement serves as a buffer against gas volatility by ensuring that all your requests have more than enough funding to go through. If your balance is below the minimum, your requests remain pending for up to 24 hours before they expire. After you add sufficient LINK to a subscription, pending requests automatically process as long as they have not expired.

A user can send a big number of transactions to consume all the LINK balance of the team account in a very little amount of time because a request can be executed in 3 blocks, (uint16 public requestConfirmations = 3), less than 5 minutes.

    function fulfillRandomWords(uint256 _requestId, uint256[] memory _randomWords) internal override {
        gencoreContract.setTokenHash(tokenIdToCollection[requestToToken[_requestId]], requestToToken[_requestId], bytes32(abi.encodePacked(_randomWords,requestToToken[_requestId])));
        emit RequestFulfilled(_requestId, _randomWords);
    }

Notes

I think this issue can be considered as High because the randomness of the token generation can be broken by anyone in simple way. And a malicious user can force the team to spend all their availables LINK tokens to request useless random transactions.

Tools Used

Manual Review, Chainlink Docs

Recommended Mitigation Steps

Like it's explained in the Security Considerations of the Chainlink VRF docs, you have to respect the procces explained: https://docs.chain.link/vrf/v2/security#dont-accept-bidsbetsinputs-after-you-have-made-a-randomness-request

Consider the example of a contract that mints a random NFT in response to a user's actions.

The contract should:

1. Record whatever actions of the user may affect the generated NFT.
2. Stop accepting further user actions that might affect the generated NFT and issue a randomness request.
3. On randomness fulfillment, mint the NFT.

Generally speaking, whenever an outcome in your contract depends on some user-supplied inputs and randomness, the contract should not accept any additional user-supplied inputs after it submits the randomness request.

Otherwise, the cryptoeconomic security properties may be violated by an attacker that can rewrite the chain.

You can decide to call the _safeMint function on the fulfillRandomWords() callback to asure the token is not minted with zero token hash. Also create for example a tokenToRequest[status] that will be set to false or true to check the status of the requests and make the correct verification that a token can be minted. And add an Admin function to potentially unblock the situation if the fulfillRandomWords() can't be executed caused by lack of funds.

It's an example and implement a solution without introduce new bugs needs to be discussed. If I can help, I'll be glad to do so.

Assessed type

Oracle

c4-pre-sort commented 12 months ago

141345 marked the issue as primary issue

c4-sponsor commented 11 months ago

a2rocket (sponsor) disputed

a2rocket commented 11 months ago

not clear how the randomness can be broke, the randomness is returned after a specific number of blocks and once set it cannot be set again so the other transactions will fail. Also by fixing reentrancy attacks reported already we can avoid someone calling the mint function continuously.

c4-pre-sort commented 11 months ago

141345 marked the issue as sufficient quality report

c4-judge commented 11 months ago

alex-ppg marked the issue as duplicate of #1464

c4-judge commented 11 months ago

alex-ppg marked the issue as unsatisfactory: Invalid

AxelAramburu commented 11 months ago

@alex-ppg Hi Alex, thanks for your time and your professionalism in judging the contest. I want to emphasize for a malicious user can force the team to spend all their availables LINK tokens to request useless random transactions.

Like I say, a malicious user can force the minting process to revert with a contract by not implementing the onERC721Received method on it, and because the call to requestRandomWords() to the VRF Coordinator occurs before the revert, a request is sended and the team have to pay with LINK amounts. The minting process is open for any user, and the attacker can send many minting attempts to force the team to spend all their LINK tokens. And because the transactions take less than 5 minutes to be treated by the Coordinator, the team have low amount of time to react to this potential attack. In my humble opinion, this is an easy attack to carry out, costing the attacker only the price of gas.

It can be remediate by calling the _safeMint() before the request to the VRF Coordinator.

Thanks for your time !

alex-ppg commented 11 months ago

Hey @AxelAramburu, thanks for contributing! If a transaction issues a revert, all state changes that are part of the transaction are rolled back. This includes event emissions as well as any function calls that perform state changes in external contracts. As such, the requestRandomWords would not have been relayed if the transaction reverted on the onERC721Received hook.