code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/RNSourceController.sol#L46-L56 https://github.com/code-423n4/2023-03-wenwin/blob/main/src/RNSourceController.sol#L89-L104

Vulnerability details

The docs state that Chainlink VRF will be used as the source of randomness, whose subscription model is described here. A call is made to Chainlink's VRFCoordinatorV2 requestRandomWords function, after which a response is sent back in the form of a call to fulfillRandomWords on the requesting contract.

A balance of LINK tokens is needed in order to pay for this request. If that balance is insufficient, then no response will be sent. This can be abused to manipulate the randomness of the lottery as follows: the owner of Lottery sets the subscriptions LINK balance to 0, waits for a sufficient amount of calls to retry to be made such that they are able to call swapSource, tops up their VRF subscription LINK balance and calls retry once more, looks in the mempool for the fulfillRandomWords function, and then frontruns it with a call to swapSource if the random number result is unfavourable. This would indeed lead to fulfillRandomWords reverting because once it attempts to execute onRandomNumberReceived, the check will fail due to source being changed to a new address:

function onRandomNumberFulfilled(uint256 randomNumber) external override {
    // @audit below check will fail
    if (msg.sender != address(source)) {
        revert RandomNumberFulfillmentUnauthorized();
    }

    lastRequestFulfilled = true;
    failedSequentialAttempts = 0;
    maxFailedAttemptsReachedAt = 0;

    receiveRandomNumber(randomNumber);
}

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();
}

Impact

The Lottery owner may manipulate the randomness of the draw, favouring themselves or disadvantaging other users. For example, this could be exploited in order to prevent users from winning the jackpot, therefore protecting the solvency of the protocol. A similar issue from a separate audit can be found here.

Proof of Concept

Consider the following scenario:

Lottery owner (perhaps using a different address) buys up lottery tickets
The subscription ID has a balance of 0 LINK
The call to executeDraw and each subsequent call to retry all fail as there is insufficient LINK balance available
Before the 6th call to retry, the Lottery owner tops up the LINK balance
The request now succeeds as the balance is sufficient
Owner scouts the mempool for the fulfillRandomWords transaction and sees the random result
If the result would lead to a profit on their purchased tickets:
- they do nothing; the request is fulfilled and they receive their profit
Otherwise: -- they frontrun the transaction with a call to swapSource
- the fulfillRandomWords transaction reverts and the lottery does not settle
- after 5 more calls to retry, they call swapSource to change the source to the original Chainlink VRFCoordinatorV2 address and repeat the process

Tools Used

Manual review, Chainlink docs

Recommended Mitigation Steps

The only way to mitigate this vulnerability with the current protocol structure is to prevent the tampering of the subscription IDs LINK balance. This can be done by ensuring that the address responsible for managing the subscription is unable to withdraw LINK from the balance, by using the address of a simple smart contract which is only capable of adding funds to the subscription as the managing address.

An alternative approach would be to display the subscriptions LINK balance to the end user in some way to ensure the request will succeed, however this is insufficient since the owner can simply wait until the time of the draw (after many tickets will have been purchased) to reduce the LINK balance to 0.

c4-judge commented 1 year ago

thereksfour marked the issue as duplicate of #393