code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In RNSourceController.sol, after a random number has been repeatedly requested by a fixed number of times (i.e. maxFailedAttempts), the owner is able to call the swapSource function to change the source of randomness for the lottery.

It is important to note that for Chainlink VRF v2 to fulfill a request for a random number, the relevant Chainlink subscription balance must have a sufficient amount of LINK. If the subscription is underfunded, the randomness oracle will not return a random number.

The owner could maliciously cause their Chainlink subcription to be underfunded, and after calling retry() a sufficient number of times (none of these requests would be fulfilled due to the subscription balance being underfunded), the owner would then be able to arbitrarily set the source of randomness to any address using the swapSource function, including to a malicious contract that just returns whatever number the owner would like (with absolutely no randomness) so that they can win the jackpot, or to a contract that does not return a number at all, which would mean the current draw would never finalize and there would be no future lotteries.

This issue is distinct from the automated finding "[M-1] Centralization Risk for trusted owners" as this issue is not strictly that the owner has the right to change the randomness source if certain conditions are met, this issue is that the owner is able to unfairly and maliciously influence the conditions that give rise to its right to change the randomness source (i.e. by underfunding its Chainlink VRF v2 subscriptionID).

On the competition page, RNSourceController is described as a contract that "controls random number sources and prevents the lottery's owner from changing the source without it failing to deliver a random number for a while" and the protocol's documentation similarly says "it's important to understand that the deployer cannot change the randomness source other than in the unlikely scenario in which the Chainlink oracle fails repeatedly" so it is a high risk vulnerability that the owner is able to bypass the protections in RNSourceController that are meant to prevent the owner from arbitrarily changing the randomness source by purposefully causing the Chainlink oracle to fail to return a random number. The lottery is the core feature of the protocol, so the lottery being rigged or stopped from running are very serious consequences, further justifying the high level.

Proof of Concept

Attack can be carried out as follows:

Owner buys a lottery ticket
Owner subscriptionID is left with 0 LINK balance
After a draw has been executed, the Owner calls retry() a number of times with a sufficient delay between each request, so that they are able to call swapSource()
The Owner calls swapSource() and changes the source of randomness to a malicious contract that is not random at all, and when that contract receives a request for a random number, it will simply return a fixed number that will cause the Owner's ticket to win the jackpot. Alternatively, the malicious contract can be set to not return a number, DOSing the lottery contract.

Tools Used

Manual review, Chainlink documentation

Recommended Mitigation Steps

Consider adding more robust governance requirements around the change of the randomness source, given how critical it is to the operation of the lottery. For example, if the randomness source needs to be changed due to a legitimate long lasting issue with Chainlink VRF v2, after the owner changes the randomness source, there could be a fixed cooldown period implemented before that change becomes effective during which users could refund tickets bought for that draw if they thought the new randomness source was malicious or unfair.

thereksfour commented 1 year ago

Centralized risk, basically equivalent to M-01 in autofind, invalid.

c4-judge commented 1 year ago

thereksfour marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-03-wenwin-findings