Wenwin contest - Aymen0909's results

QA Report

Summary

Findings

1- fulfillRandomWords should never revert :

Risk : Low

The function fulfillRandomWords (in the VRFv2RNSource) contract should never revert according to the Chainlink documentation, as in that case the VRF service will not attempt to call it a second time.

Further more the check statement in the fulfillRandomWords function seems to be of no effect as the actual randomness call to the function requestRandomness specify in the given arguments that only one random word is requested, so the check should be removed.

Proof of Concept

This issue occurs in the following instance :

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/VRFv2RNSource.sol#L32-L38

Mitigation

The check statement inside the fulfillRandomWords should be removed to avoid the revert of the call.

2- randomness should not be requested multiple times:

Risk : Low

In the RNSourceController contract a logic is implemented to re-request randomness from the Chainlink VRF in case the first calls failed but this is warned against in the Chainlink documentation and can potentially introduce an attack surfaces that can impact the protocol working.

This issue should be reviewed by the protocol devs to make sure that their concepts work perfectly and does not have weaknesses or attacking areas.

3- getMinimumEligibleReferralsFactorCalculation should use <= instead of <:

Risk : Low

In the RNSourceController contract the function getMinimumEligibleReferralsFactorCalculation should use the operator <= in the if-statements comparison instead of the operator < as it is highlighted in the referrals documentation, even though the difference between the two operators is very small in this case this can still cause some referrer to get the wrong factor calculation and will create some confusion.

4- Immutable state variables lack zero address checks :

Constructors should check the values written in an immutable state variables(address) is not the zero address (address(0))

Risk : Low

Proof of Concept

Instances include:

File: StakedTokenLock.sol Line 18

stakedToken = IStaking(_stakedToken);

File: RNSourceBase.sol Line 12

authorizedConsumer = _authorizedConsumer;

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

5- require should be used instead of assert :

Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction’s available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that “The assert function creates an error of type Panic(uint256). … Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix”.

Risk : Non critical

Proof of Concept

Instances include:

File: LotterySetup.sol Line 147

assert(initialPot > 0);

File: TicketUtils.sol Line 99

assert((winTier <= selectionSize) && (intersection == uint256(0)));

Mitigation

Replace the assert checks aforementioned with require statements.

6- Named return variables not used anywhere in the function :

When Named return variable are declared they should be used inside the function instead of the return statement or if not they should be removed to avoid confusion.

Risk : Non critical

Proof of Concept

Instances include:

File: LotterySetup.sol Line 120

returns (uint256 amount)

File: ReferralSystem.sol Line 115

returns (uint256 minimumEligible)

File: ReferralSystem.sol Line 156

returns (uint256 rewards)

File: ReferralSystem.sol Line 161

returns (uint256 rewards)

File: TicketUtils.sol Line 24

returns (bool isValid)

Mitigation

Either use the named return variables inplace of the return statement or remove them.