Wenwin contest - slvDev's results

[L-01] Consider using OpenZeppelin’s SafeCast library to prevent unexpected overflows when casting from uint256

The LotterySetup contract includes a function called packFixedRewards, which takes an argument called rewards. The rewards argument is an array of type uint256.

When the function calculates the reward, it divides rewards[winTier] by divisor and downcasts the result to uint16.

File: src\LotterySetup.sol

function packFixedRewards(uint256[] memory rewards) private view returns (uint256 packed) {
						...
            uint16 reward = uint16(rewards[winTier] / divisor);
            ...
    }

Recommended Mitigation Steps:

Consider using OpenZeppelin’s SafeCast library to prevent unexpected overflows when casting from uint256.

[L-02] All functions in the library PercentageMath can revert with overflow

Since this library was developed to use on uint256 but in the function's body we have uin256 multiplication which potentially can overflow. Consider creating a helper contract with functions that have input validation or function params that cant revert with overflow after multiplication eg

function getPercentage(uint128 number, uint128 percentage) internal pure returns (uint256 result) {
        return number * percentage / PERCENTAGE_BASE;
    }

[L-03] _safeMint() should be used rather than _mint()

The use of _mint() function is discouraged and it is recommended to use _safeMint() function instead. _safeMint() ensures that the recipient is either an EOA or implements the IERC721Receiver interface, making it a more secure option for minting new tokens.

File: src\Ticket.sol

function mint(address to, uint128 drawId, uint120 combination) internal returns (uint256 ticketId) {
        ticketId = nextTicketId++;
        ticketsInfo[ticketId] = TicketInfo(drawId, combination, false);
        _mint(to, ticketId);
    }

[L-04] Missing checks for address(0x0) when assigning values to address state variables

Although this check is present in other contracts.

File: src\staking\StakedTokenLock.sol

IStaking public immutable override stakedToken;
IERC20 public immutable override rewardsToken;
...

constructor(address _stakedToken, uint256 _depositDeadline, uint256 _lockDuration) {
    ...
    stakedToken = IStaking(_stakedToken);
    rewardsToken = stakedToken.rewardsToken();
		...
}

File: src\RNSourceBase.sol

address internal immutable authorizedConsumer;
...
constructor(address _authorizedConsumer) {
    authorizedConsumer = _authorizedConsumer;
}

[Q-01] Remove unused interfaces

IReferralSystemDynamic.sol does not seem to be used anywhere, as there are no imports or contracts that inherit from it.

[Q-02] Don’t use the latest Solidity version unless it’s necessary

It is generally not recommended to use the latest version of Solidity unless it is necessary, because new versions may contain undiscovered bugs or other issues that could affect the functionality or security of your smart contract. Instead, it is recommended to use a stable and well-tested version of Solidity that is known to be reliable and secure.

[G-01] Use constants instead of type(uintx).max

Using type(uint16).max and similar operations consumes more gas during both the distribution process and for each transaction compared to using constants.

File: src\LotterySetup.sol

function fixedReward(uint8 winTier) public view override returns (uint256 amount) {
        ...
            uint256 mask = uint256(type(uint16).max) << (winTier * 16);
        ...
    }

[G-02] Using storage instead of memory for structs/arrays saves gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declaring the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.

File: src\ReferralSystem.sol

function claimPerDraw(uint128 drawId) private returns (uint256 claimedReward) {
        ... 
        UnclaimedTicketsData memory _unclaimedTickets = unclaimedTickets[drawId][msg.sender];
        ...
    }

File: src\Lottery.sol

function claimable(uint256 ticketId) external view override returns (uint256 claimableAmount, uint8 winTier) {
        TicketInfo memory ticketInfo = ticketsInfo[ticketId];
        ...
    }

[G-03] Using calldata in function args saves gas

When a function argument is declared with the calldata keyword, the function can access the argument's data directly from the transaction's input data, which is already stored in the contract's memory.

File: src\ReferralSystem.sol

function claimReferralReward(uint128[] memory drawIds) external override returns (uint256 claimedReward) 

[G-04] Use nested if and, avoid multiple check combinations

Using nested is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.

File: src\staking\StakedTokenLock.sol

function withdraw(uint256 amount) external override onlyOwner {
        ...
        if (block.timestamp > depositDeadline && block.timestamp < depositDeadline + lockDuration)
        ...
    }

File: src\staking\StakedTokenLock.sol

function withdraw(uint256 amount) external override onlyOwner {
        ...
        if (block.timestamp > depositDeadline && block.timestamp < depositDeadline + lockDuration) {
        ...
    }

[G-05] Functions guaranteed to revert_ when called by normal users can be marked payable

If a function modifier or require such as onlyOwner-admin is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

File: src\staking\StakedTokenLock.sol

function deposit(uint256 amount) external override onlyOwner 
function withdraw(uint256 amount) external override onlyOwner 

File: src\RNSourceController.sol
function initSource(IRNSource rnSource) external override onlyOwner 
function swapSource(IRNSource newSource) external override onlyOwner 

[G-06] Setting the constructor to payable

You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 13 gas on deployment with no security risks.

File: src/LotteryToken.sol
constructor() ERC20("Wenwin Lottery", "LOT") 

File: src/VRFv2RNSource.sol	
constructor(address _authorizedConsumer, address _linkAddress, address _wrapperAddress, uint16 _requestConfirmations, uint32 _callbackGasLimit)

File: src/staking/StakedTokenLock.sol
constructor(address _stakedToken, uint256 _depositDeadline, uint256 _lockDuration)

File: src/staking/Staking.sol
constructor(ILottery _lottery, IERC20 _rewardsToken, IERC20 _stakingToken, string memory name, string memory symbol) ERC20(name, symbol)

File: src/LotterySetup.sol
constructor(LotterySetupParams memory lotterySetupParams)

File: src/Lottery.sol
constructor(LotterySetupParams memory lotterySetupParams,uint256 playerRewardFirstDraw,uint256 playerRewardDecreasePerDraw,uint256[] memory rewardsToReferrersPerDraw,uint256 maxRNFailedAttempts,uint256 maxRNRequestDelay)	

File: src/Ticket.sol
constructor() ERC721("Wenwin Lottery Ticket", "WLT")

File: src/RNSourceBase.sol
constructor(address _authorizedConsumer) 

File: src/RNSourceController.sol
constructor(uint256 _maxFailedAttempts, uint256 _maxRequestDelay)

File: src/ReferralSystem.sol
constructor(uint256 _playerRewardFirstDraw,uint256 _playerRewardDecreasePerDraw, uint256[] memory _rewardsToReferrersPerDraw)

[G-07] x += y (x -= y) costs more gas than x = x + y (x = x - y) for state variables

File: src\staking\StakedTokenLock.sol

function deposit(uint256 amount) external override onlyOwner {
		...
    depositedBalance += amount;
		...
}

function withdraw(uint256 amount) external override onlyOwner {
    ...
    depositedBalance -= amount;
		...
}

[G-08] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require or if statement

This will stop the check for overflow and underflow so it will save gas.

function returnUnclaimedJackpotToThePot() private {
      if (currentDraw >= LotteryMath.DRAWS_PER_YEAR) {
          uint128 drawId = currentDraw - LotteryMath.DRAWS_PER_YEAR;
          ...
		}
}

[G-09] internal functions only called once can be inlined to save gas

File: src\Lottery.sol

function requireFinishedDraw(uint128 drawId) internal view override 
function mintNativeTokens(address mintTo, uint256 amount) internal override 

File: src\LotterySetup.sol

function _baseJackpot(uint256 _initialPot) internal view returns (uint256)