Wenwin contest - 0xAgro's results

QA Report

Finding Summary

Low Severity

1. assert Used Over require
2. Unchecked Cast May Overflow
3. Early Use of Compiler Version
4. Lack of Events For Critical Functions
5. fulfillRandomWords Reverts
6. ERC20 Decimals Assumption

Non-Critical

1. Underscore Notation Not Used or Not Used Consistently
2. Power of Ten Literal Not In Scientific Notation
3. Inconsistent Named Returns
4. Lack of NatSpec Contract Headers
5. Named Returns Not Returning Named Variable
6. Magic Numbers Used
7. Inconsistent Comment Capitalization
8. Inconsistent Trailing Period
9. Unnecessary Headers
10. ERC20 Token Recovery

Style Guide Violations

1. Order of Functions
2. Order of Layout

Low Severity

1. assert Used Over require

assert should only be used in tests. Consider changing all occurrences of assert to require. Prior to Solidity 0.8 require will refund all remaining gas whereas assert will not. Even after Solidity 0.8 assert will result in a panic which should not occur in production code. As stated in the Solidity Documentation:

Properly functioning code should never create a Panic.

/src/LotterySetup.sol

147:	assert(initialPot > 0);

/src/TicketUtils.sol

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

2. Unchecked Cast May Overflow

As of Solidity 0.8 overflows are handled automatically; however, not for casting. For example uint32(4294967300) will result in 4 without reversion. Consider using OpenZepplin's SafeCast library.

/src/LotterySetup.sol

170:	uint16 reward = uint16(rewards[winTier] / divisor);

/src/Lottery.sol

275:	currentNetProfit += int256(unclaimedJackpotTickets * winAmount[drawId][selectionSize]);

/src/ReferralSystem.sol

69:	unclaimedTickets[currentDraw][referrer].referrerTicketCount += uint128(numberOfTickets);
71:	unclaimedTickets[currentDraw][player].playerTicketCount += uint128(numberOfTickets);

/src/PercentageMath.sol

23:	return number * int256(percentage) / int256(PERCENTAGE_BASE);

/src/TicketUtils.sol

58:	numbers[i] = uint8(randomNumber % currentSelectionCount);
96:	winTier += uint8(intersection & uint120(1));

/src/LotteryMath.sol

48:	newProfit = oldProfit + int256(ticketsSalesToPot);
55:	newProfit -= int256(expectedRewardsOut);
64:	excessPotInt -= int256(fixedJackpotSize);

3. Early Use of Compiler Version

Solidity 0.8.19 is used in all contracts except for VRFv2RNSource.sol, StakedTokenLock.sol, and IVRFv2RNSource.sol. Solidity 0.8.19 as of the start of the contest, is less than 2 weeks old.

By using a compiler version early you are volunteering as guinea pigs for potential bugs when it is not necessary. As an example in Solidity 0.8.13, a compiler bug was found 81 days after the release announcement (see links).

Consider downgrading contracts of version less 0.8.19 to a more battle-tested Solidity version.

4. Lack of Events For Critical Functions

Although Transfer events are emitted, the following functions should have explicit events as they offer transparency to the user:

• deposit, withdraw, getReward.

A good example of event emission of a critical function can be seen here.

5. fulfillRandomWords Reverts

The chainlink documentation states:

If your fulfillRandomWords() implementation reverts, the VRF service will not attempt to call it a second time. Make sure your contract logic does not revert. Consider simply storing the randomness and taking more complex follow-on actions in separate contract calls made by you, your users, or an Automation Node.

The current implementation of fulfillRandomWords can revert, seen below:

32:    function fulfillRandomWords(uint256 requestId, uint256[] memory randomWords) internal override {
33:        if (randomWords.length != 1) {
34:             revert WrongRandomNumberCountReceived(requestId, randomWords.length);
35:        }
36:	
37:        fulfill(requestId, randomWords[0]);
38:    }

6. ERC20 Decimals Assumption

EIP20 states in regard to the decimals method:

OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present.

The protocol assumes the decimals method is present which may affect the system if whole tokens are used.

/src/LotterySetup.sol

79:	uint256 tokenUnit = 10 ** IERC20Metadata(address(lotterySetupParams.token)).decimals();
128:	return extracted * (10 ** (IERC20Metadata(address(rewardToken)).decimals() - 1));
168:	uint256 divisor = 10 ** (IERC20Metadata(address(rewardToken)).decimals() - 1);

Non-Critical

1. Underscore Notation Not Used or Not Used Consistently

Consider using underscore notation to help with contract readability (Ex. 23453 -> 23_453).

/src/ReferralSystem.sol

129:	return 5000;

2. Power of Ten Literal Not In Scientific Notation

Power of ten literals > 1e2 are easier to read when expressed in scientific notation. Consider expressing large powers of ten in scientific notation.

/src/PercentageMath.sol

11:	uint256 public constant ONE_PERCENT = 1000;

3. Inconsistent Named Returns

All functions in the code based use named returns except for _baseJackpot in LotterySetup.sol. Consider using a named return in order to maintain code style.

4. Lack of NatSpec Contract Headers

No contracts in scope have a fully annotated NatSpec contract header (@title, @author, etc. see here for reference). Contracts have a partial NatSpec header like this or no header like this. Consider adding properly annotated NatSpec headers.

5. Named Returns Not Returning Named Variable

Using named returns may help developers understand what is being returned, but this should be in a @return tag. There is no need to confuse developers. The following functions have named returns, but still return a value.

/src/staking/Staking.sol

• rewardPerToken, earned.

/src/LotterySetup.sol

• fixedReward.

/src/Lottery.sol

• currentRewardSize, drawRewardSize.

/src/ReferralSystem.sol

• getMinimumEligibleReferralsFactorCalculation, playerRewardsPerDraw, referrerRewardsPerDraw.

/src/PercentageMath.sol

• getPercentage, getPercentageInt.

/src/TicketUtils.sol

• isValidTicket.

6. Magic Numbers Used

When setting up a lottery LotterySetup.sol there are a couple of magic numbers used in comparison. It is best practice to replace all magic numbers with constants.

51:	if (lotterySetupParams.selectionMax >= 120) {

60:	if (
61:	    lotterySetupParams.selectionSize > 16 || lotterySetupParams.selectionSize >= lotterySetupParams.selectionMax
62:	) {

Here is a good use of constants in the codebase.

7. Inconsistent Comment Capitalization

There is an inconsistency in the capitalization of comments. For example, this line is not capitilized while this line is. Consider capitilizing all comments.

8. Inconsistent Trailing Period

Some comments like this have a trailing . but other lines like this do not.

9. Unnecessary Headers

Staking.sol is the only contract in scope that has headers seen here and here. The positioning of view functions or mutative functions can be inferred by the order of layout of the contract (if the contract follows the Solidity Style Guide). Consider removing the headers in Staking.sol.

10. ERC20 Token Recovery

Consider adding a recovery function for Tokens that are accidently sent to the core contracts of the protocol.

Style Guide Violations

1. Order of Functions

The Solidity Style Guide suggests the following function order: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private.

The following contracts are not compliant (examples are only to prove the functions are out of order NOT a full description):

• Staking.sol: external functions are positioned after public functions.
• LotterySetup.sol: external functions are positioned after public functions.
• Lottery.sol: internal function is positioned after private functions.
• RNSourceController.sol: external functions are positioned after internal functions.
• ReferralSystem.sol: external functions are positioned after internal functions.

2. Order of Layout

The Solidity Style Guide suggests the following contract layout order: Type declarations, State variables, Events, Modifiers, Functions.

The following contracts are not compliant (examples are only to prove the layout are out of order NOT a full description):

• LotterySetup.sol: Modifiers are positioned after Functions (constructor).
• IRNSource.sol: State is positioned after Events.
• IStaking.sol: Events are positioned after Functions.