Attack pathways to render the protocol unusable and undermine the fairness of Lottery.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/RNSourceController.sol#L106-L120 https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/RNSourceController.sol#L60-L75 https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/RNSourceController.sol#L89-L104

Vulnerability details

Introduction

This report bundles together several issues with regard to the current implementation of Executing a draw, Requesting RNG from Chainlink and Fulfilling RNG requests. The reason to summarize several attack paths in one vulnerability report is that the discovered logic flaws and assumptions are closely interconnected and the clarity of explaining the disclosed findings will be greater.

The current implementation of Random Number Generation uses Chainlink’s V2 Direct Funding Method (https://docs.chain.link/vrf/v2/direct-funding). VRFv2RNSource.sol (inherits Chainlink’s VRFV2WrapperConsumerBase.sol) is responsible for handling requests and responses for the Lottery system. The communicator between VRFv2RNSource.sol contract and Lottery.sol is RNSourceController.sol. The ideal flow of control is the following:

1. Any user can call executeDraw() in Lottery.sol assuming that the current draw is past the scheduled time for registering tickets.
2. executeDraw() puts the system in the state of drawExecutionInProgress = true and calls requestRandomNumber().
3. in RNSourceController.sol - requestRandomNumber() checks if the previous draw was completed and calls requestRandomNumberFromSource().
4. requestRandomNumberFromSource() records the timestamp of the request in lastRequestTimestamp = block.timestamp and sets lastRequestFulfilled = false i.e executeDraw() cannot be called until the draw is finished. Lastly source.requestRandomNumber() is invoked.
5. Now source.requestRandomNumber() calls requestRandomnessFromUnderlyingSource() and that subsequently calls requestRandomness() to generate a RN from Chainlink VRF.
6. Several blocks later Chainlink VRF has verified a RN and sends a callback call to fulfillRandomWords() that calls fulfill(), which calls onRandomNumberFulfilled() in the RNSourceController.sol that sets lastRequestFulfilled = true and lastly receiveRandomNumber(randomNumber) is invoked in Lottery.sol that sets drawExecutionInProgress = false and starts a new draw (increments currentDraw state variable).

Issue #1 - An attacker can leave the protocol in a "drawing" state for extended period of time

The culprit for this issue is the implementation of requestRandomNumberFromSource() in RNSourceController.sol. After lastRequestFulfilled = false the invocation to VRFv2RNSource.sol is done in a try{} catch{} block -

        lastRequestTimestamp = block.timestamp;
        lastRequestFulfilled = false;

        try source.requestRandomNumber() {
            emit SuccessfulRNRequest(source);
        } catch Error(string memory reason) {
            emit FailedRNRequest(source, bytes(reason));
        } catch (bytes memory reason) {
            emit FailedRNRequest(source, reason);
        }
    }

This is very problematic due to how try{} catch{} works - OpenZeppelin article. If the request to Chainlink VRF fails at any point then execution of the above block will not revert but will continue in the catch{} statements only emitting an event and leaving RNSourceController in the state lastRequestFulfilled = false and triggering the maxRequestDelay (currently 5 hours) until retry() becomes available to call to retry sending a RN request. This turns out to be dangerous since there is a trivial way of making Chainlink VRF revert - simply not supplying enough gas for the transaction either initially in calling executeDraw() or subsequently in retry() invocations with the attacker front-running the malicious transaction.

Proof of Concept

1. Ticket registration closes, attacker calls `executeDraw()` with insufficient gas and Lottery is put in the Executing Draw State (drawExecutionInProgress = true).
2. Attacker front-runs the transaction if there are other `executeDraw()` transactions.
3. `RNSourceController.sol` calls `VRFv2RNSource.so` in a `try{} catch{}` block, VRF transaction reverts and `lastRequestFulfilled` remains equal to `false`.
4. After “maxRequestDelay” time has past retry() becomes available that relies on the same `try{} catch{}` block in `requestRandomNumberFromSource()`.
5. Attacker calls `retry()` with insufficient gas and front-runs the transaction if there are other `retry()` transactions.
6. Attacker repeats steps 4 and 5 leaving the system in a Drawing state for extended period of time (5 hours for every retry() in the example implementation).

Moreover, the attacker doesn’t have any incentive to deposit LINK himself since VRF will also revert on insufficient LINK tokens. This Proof of Concept was also implemented and confirmed in a Remix environment, tested on the Ethereum Sepolia test network. A video walk-through can be provided on my behalf if requested by judge or sponsors.

Impact

System is left for extended period of time in “Drawing” state without the possibility to execute further draws, user experience is damaged significantly.

Comment

Building upon Issue #1, an obvious observation arises - there is the swapSource() method that becomes available ( only to the owner ) after a predefined number of failed retry() invocations - maxFailedAttempts. Therefore, potentially, admins of the protocol could replace the VRF solution with a better one that is resistant to the try catch exploit? It turns out that the current implementation of swapSource() introduces a new exploit that breaks the fairness of the protocol and an edge case could even be constructed that leads to an attacker stealing a jackpot.

Issue #2 - Undermining the fairness of the protocol in swapSource() and possibilities for stealing a jackpot

This issue will demonstrate how the current implementation of swapSource() and retry() goes directly against Chainlink's Security Consideration of Not re-requesting randomness (https://docs.chain.link/vrf/v2/security#do-not-re-request-randomness).

Note

For clarity it is assumed that after swapSource() the new source would be Chainlink Subscription Method implementation and I would refer to it again as Chainlink VRF since this was the initial design decision, however it is of no consequence what the new VRF is.

The Exploit

The swapSource() method can be successfully called if 2 important boolean checks are true. notEnoughRetryInvocations - makes sure that there were maxFailedAttempts failed requests for a RN. notEnoughTimeReachingMaxFailedAttempts - makes sure that maxRequestDelay amount of time has passed since the timestamp for reaching maxFailedAttempts was recorded in maxFailedAttemptsReachedAt i.e sufficient time has passed since the last retry() invocation. The most important detail to note here is that the swapSource() function does not rely on lastRequestTimestamp to check whether maxRequestDelay has passed since the last RN request.

    function swapSource(IRNSource newSource) external override onlyOwner {
        if (address(newSource) == address(0)) {
            revert RNSourceZeroAddress();
        }
        bool notEnoughRetryInvocations = failedSequentialAttempts < maxFailedAttempts;
        bool notEnoughTimeReachingMaxFailedAttempts = block.timestamp < maxFailedAttemptsReachedAt + maxRequestDelay;
        if (notEnoughRetryInvocations || notEnoughTimeReachingMaxFailedAttempts) {
            revert NotEnoughFailedAttempts();
        }
        source = newSource;
        failedSequentialAttempts = 0;
        maxFailedAttemptsReachedAt = 0;

        emit SourceSet(newSource);
        requestRandomNumberFromSource();
    }

The critical bug resides in the retry() method. maxFailedAttemptsReachedAt is ONLY updated when failedAttempts == maxFailedAttempts - notice again the strict equality - meaning that maxFailedAttemptsReachedAt won't be updated if there are more retry() invocations after failedAttempts == maxFailedAttempts. This means that after the point of time when the last failed retry() sets maxFailedAttemptsReachedAt and the maxRequestDelay time passes - retry() and swapSource() (in that exact order) can be called simultaneously.

        uint256 failedAttempts = ++failedSequentialAttempts;
        if (failedAttempts == maxFailedAttempts) {
            maxFailedAttemptsReachedAt = block.timestamp;
        }

The attacker would monitor the transaction mempool for the swapSource() invocation and front-run a retry() invocation before the swapSource transaction. Now we have two separate - seemingly at the same time - calls to requestRandomNumberFromSource() and again to note that the retry() call will update lastRequestTimestamp = block.timestamp but it will not update maxFailedAttemptsReachedAt since now failedAttempts > maxFailedAttempts and as presented swapSource() does not rely on lastRequestTimestamp which makes all of this possible. Now we have two requests at the same time to VRFv2RNSource.sol and in turn Chainlink VRF. Chainlink VRF will in turn send 2 callback calls each containing a random number and the callbacks can be inspected by the attacker and in turn he will front-run the RN response that favours him greater thus undermining the fairness of the protocol.

Proof of Concept

1. `retry()` is called enough times to reach `maxFailedAttempts`, attacker starts monitoring the mempool for the `swapSource()` call.
2. Admin decides to swap Source. Admin waits for `maxRequestDelay` time to pass and calls `swapSource()`.
3. Attacker notices the `swapSource()` call and front-runs a `retry()` call before the `swapSource()` invocation.
4. The introduced code bug displays that `swapSource()` is not invalidated by the front-ran `retry()` and both `retry()` and `swapSource()` request a random number from Chainlink VRF.
5. Attacker now scans the mempool for the callbacks from the requests to VRF, inspects the random numbers and front-runs the transaction with the random number that favors him.

Impact

Re-requesting randomness is achieved when swapping sources of randomness. Fairness of protocol is undermined.

Note

Recently published finding by warden Trust discusses a very similar attack path that has to do with more than 1 VRF callbacks residing in the mempool (https://code4rena.com/reports/2022-12-forgeries#h-02-draw-organizer-can-rig-the-draw-to-favor-certain-participants-such-as-their-own-account).

Edge Case - stealing a jackpot when swapping randomness Source

The Wenwin protocol smart contracts are build such that various configurations of the system can be deployed. The provided documentation gives example with a weekly draw, however drawPeriod in LotterySetup.sol could be any value. A lottery that is deployed with drawPeriod of for example 1 hour rather than days can be much more susceptible to re-requesting randomness. Similarly to Issue #2 an attacker would anticipate a swapSource() call to front-run it with retry() call and generate 2 RN requests. Now the attacker would use another front-running technique - Supression, also called block-stuffing, an attack that delays a transaction from being executed (reference in link section). The attacker would now let one of the generated RN callback requests return to the contract and reach the receiveRandomNumber() in Lottery.sol and let the protocol complete the current draw and return the system back in a state that can continue with the next draw - all of that while suppressing the second RN callback request. The attacker would register a ticket with the combination generated from the Random Number in the suppressed callback request and when executeDraw() is triggered he would then front-run the "floating" callback request to be picked first by miners therefore calling 'fulfillRandomWords()' with the known RN and winning the jackpot.

Relevant links

Consensys front-running attacks - (https://consensys.github.io/smart-contract-best-practices/attacks/frontrunning/#suppression) Medium article on Block Stuffing and Fomo3D - (https://medium.com/hackernoon/the-anatomy-of-a-block-stuffing-attack-a488698732ae)

Proof of Concept

1. Assume Lottery configuration with a short `drawPeriod` e.g 1 hour.
2. `retry()` is called enough times to reach `maxFailedAttempts`, attacker starts monitoring the mempool for the `swapSource()` call.
3. Admin decides to swap Source. Admin waits for `maxRequestDelay` time to pass and calls `swapSource()`.
4. Attacker notices the `swapSource()` call and front-runs a `retry()` call before the `swapSource()` invocation thus achieving 2 callback RN requests as in Issue #2 PoC.
5. Attacker uses front-running suppression after one of the callback requests is picked up by the protocol and therefore current draw is finalized.  
6. Attacker registers a ticket with the winning combination generated from the suppressed RN.
7. After `drawPeriod` is past attacker or other user calls `executeDraw()`.
8. Attacker front-runs the "suppressed RN" to be picked by miners first.
9. 'fulfillRandomWords()' is called with the RN known by the attacker thus winning a jackpot.

Impact

Fairness of the protocol is undermined when swapping Sources in Lottery configurations with short drawPeriod. Unfair win of a jackpot.

Tools Used

Manual Inspection Issue #1 PoC implementation in Remix tested against Sepolia test network Chainlink documentation VRF, VRF security practices, Direct funding OpenZeppelin try{} catch{} article Consenys front-running attacks link ImmuneBytes front-running attacks article Medium article on Suppression link Previous codearena finding by warden Trust link

Recommended Mitigation Steps

Refactor the try{} catch{} in requestRandomNumberFromSource() in RNSourceController.sol. Replace failedAttempts == maxFailedAttempts with failedAttempts >= maxFailedAttempts in retry() in RNSourceController.sol. Evaluate centralization risks that spawn from the fact that only owners can decide on what a new source of randomness can be i.e swapSource(). Ensure that when swapping sources the new Source doesn't introduce new potential attack vectors, I would suggest reading warden's Trust report from Forgeries competition that displays potential attack vectors with retry-like functionality when using Chainlink VRF Subscription Method. Ensure all Security Considerations in Chainlink VRF documentation are met.

Final note

I would be glad to be invited to participate in any discussion that arises from the disclosed findings.

[c4-judge]

thereksfour marked the issue as primary issue

[rand0c0des]

I would say this is an issue, but would say it is a medium risk.

[c4-sponsor]

rand0c0des marked the issue as disagree with severity

[thereksfour]

After a careful reading, I will consider this report as two medium issues and will link with the corresponding duplicates

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[c4-judge]

thereksfour marked the issue as grade-c

[c4-sponsor]

rand0c0des marked the issue as sponsor confirmed