Forgeries contest - Trust's results

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L173 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L203

Vulnerability details

Description

In RandomDraw, host can call startDraw() or redraw() to request a Chainlink random number, which will be used to select the winning user. They may then collect the prize NFT using winnerClaimNFT().

The issue is that in the two draw() functions, it is never checked that the current time or the time when oracle returns the random number, is less than the recoverTimelock time. When recoverTimelock is passed, owner can always claim back the NFT using lastResortTimelockOwnerClaimNFT. Therefore, host can delay calling draw or redraw until moments before recoverTimelock. If they like the selected winner, they let them pick up the item. If they don't, they can immediately send a TX to pick up the NFT using lastResortTimelockOwnerClaimNFT. This creates an unfair illusion of fair draw which users cannot be aware of. It harms the integrity of the protocol.

Impact

Draw organizer can time draws so that user's have the illusion of fair random, but draw can be cancelled.

Tools Used

Manual audit

Recommended Mitigation Steps

In both draw() functions, make sure the remaining time until recoveryTimelock is enough for Chainlink answer + sensible user NFT pickup time.

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L83

Vulnerability details

Description

In RandomDraw, the host initiates a draw using startDraw() or redraw() if the redraw draw expiry has passed. Actual use of Chainlink oracle is done in _requestRoll:

request.currentChainlinkRequestId = coordinator.requestRandomWords({
    keyHash: settings.keyHash,
    subId: settings.subscriptionId,
    minimumRequestConfirmations: minimumRequestConfirmations,
    callbackGasLimit: callbackGasLimit,
    numWords: wordsRequested
});

Use of subscription API is explained well here. Chainlink VRFCoordinatorV2 is called with requestRandomWords() and emits a random request. After minimumRequestConfirmations blocks, an oracle VRF node replies to the coordinator with a provable random, which supplies the random to the requesting contract via fulfillRandomWords() call. It is important to note the role of subscription ID. This ID maps to the subscription charged for the request, in LINK tokens. In our contract, the raffle host supplies their subscription ID as a parameter. Sufficient balance check of the request ID is not checked at request-time, but rather checked in Chainlink node code as well as on-chain by VRFCoordinator when the request is satisfied. In the scenario where the subscriptionID lacks funds, there will be a period of 24 hours when user can top up the account and random response will be sent:

"Each subscription must maintain a minimum balance to fund requests from consuming contracts. If your balance is below that 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."

The reason this is extremely interesting is because as soon as redraws are possible, the random response can no longer be treated as fair. Indeed, Draw host can wait until redraw cooldown passed (e.g. 1 hour), and only then fund the subscriptionID. At this point, Chainlink node will send a TX with the random response. If host likes the response (i.e. the draw winner), they will not interfere. If they don't like the response, they can simply frontrun the Chainlink TX with a redraw() call. A redraw will create a new random request and discard the old requestId so the previous request will never be accepted.

function fulfillRandomWords(
    uint256 _requestId,
    uint256[] memory _randomWords
) internal override {
    // Validate request ID
	  // <---------------- swap currentChainlinkRequestId --->
    if (_requestId != request.currentChainlinkRequestId) {
        revert REQUEST_DOES_NOT_MATCH_CURRENT_ID();
    }
	...
}

//<------ redraw swaps currentChainlinkRequestId --->
request.currentChainlinkRequestId = coordinator.requestRandomWords({
    keyHash: settings.keyHash,
    subId: settings.subscriptionId,
    minimumRequestConfirmations: minimumRequestConfirmations,
    callbackGasLimit: callbackGasLimit,
    numWords: wordsRequested
});

Chainlink docs warn against this usage pattern of the VRF -"Don’t accept bids/bets/inputs after you have made a randomness request". In this instance, a low subscription balance allows the host to invalidate the assumption that 1 hour redraw cooldown is enough to guarantee Chainlink answer has been received.

Impact

Draw organizer can rig the draw to favor certain participants such as their own account.

Proof of Concept

Owner offers a BAYC NFT for holders of their NFT collection X. Out of 10,000 tokenIDs, owner has 5,000 Xs. Rest belong to retail users.

1. owner subscriptionID is left with 0 LINK balance in coordinator
2. redraw time is set to 2 hours
3. owner calls startDraw() which will initiate a Chainlink request
4. owner waits for 2 hours and then tops up their subscriptionID with sufficient LINK
5. owner scans the mempool for fulfillRandomWords()
6. If the raffle winner is tokenID < 5000, it is owner's token
   1. Let fulfill execute and pick up the reward
7. If tokenID >= 5000
   1. Call redraw()
   2. fulfill will revert because of requestId mismatch
8. Owner has 75% of claiming the NFT instead of 50%

Note that Forgeries draws are presumably intended as incentives for speculators to buy NFTs from specific collections. Without having a fair shot at receiving rewards from raffles, these NFTs user buys could be worthless. Another way to look at it is that the impact is theft of yield, as host can freely decrease the probability that a token will be chosen for rewards with this method.

Also, I did not categorize it as centralization risk as the counterparty is not Forgeries but rather some unknown third-party host which offers an NFT incentive program. It is a similar situation to the distinction made between 1st party and 3rd party projects here

Tools Used

Manual audit Chainlink docs Chainlink co-ordinator code

Recommended Mitigation Steps

The root cause is that Chainlink response can arrive up to 24 hours from the most request is dispatched, while redraw cooldown can be 1 hour+. The best fix would be to enforce minimum cooldown of 24 hours.

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L33

Vulnerability details

Description

In VRFNFtRandomDraw.sol initialize(), the MONTH_IN_SECONDS variable is used to validate two values:

• configured time between redraws is under 1 month
• recoverTimelock (when NFT can be returned to owner) is less than 1 year in the future

if (_settings.drawBufferTime > MONTH_IN_SECONDS) {
    revert REDRAW_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_MONTH();
}
...
if (
    _settings.recoverTimelock >
    block.timestamp + (MONTH_IN_SECONDS * 12)
) {
    revert RECOVER_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_YEAR();
}

The issue is that MONTH_IN_SECONDS is calculated incorrectly:

/// @dev 60 seconds in a min, 60 mins in an hour
uint256 immutable HOUR_IN_SECONDS = 60 * 60;
/// @dev 24 hours in a day 7 days in a week
uint256 immutable WEEK_IN_SECONDS = (3600 * 24 * 7);
// @dev about 30 days in a month
uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;

MONTH_IN_SECONDS multiplies by 7 incorrectly, as it was copied from WEEK_IN_SECONDS. Therefore, actual seconds calculated is equivalent of 7 months. Therefore, recoverTimelock can be up to a non-sensible value of 7 years, and re-draws every up to 7 months.

Impact

Protocol safeguards for time durations are skewed by a factor of 7. Protocol may potentially lock NFT for period of 7 years.

Tools Used

Manual audit

Recommended Mitigation Steps

Fix MONTH_IN_SECONDS calculation: uint256 immutable MONTH_IN_SECONDS = (3600 * 24) * 30;