Wenwin contest - 0x1f8b's results

• Low
  • 1. Integer overflow by unsafe casting
  • 2. Lack of checks address0
  • 3. Lack of checks supportsInterface
  • 4. Lack of checks integer ranges
  • 5. Incompatible with tokens without decimals
  • 6. The referer should never be yourself
  • 7. Centralized risk
• Non critical
  • 8. Unify return criteria
  • 9. Improve naming

Low

1. Integer overflow by unsafe casting

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Recommendation:

Use a safeCast from Open Zeppelin.

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

Affected source code:

• PercentageMath.sol:23
• LotteryMath.sol:48
• LotteryMath.sol:55
• LotteryMath.sol:64-65
• ReferralSystem.sol:69-71

2. Lack of checks address(0)

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• RNSourceBase.sol:12

3. Lack of checks supportsInterface

The EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.

Reference:

• https://eips.ethereum.org/EIPS/eip-165

Affected source code:

• RNSourceController.sol:78
• RNSourceController.sol:90
• StakedTokenLock.sol:18
• Staking.sol:31

4. Lack of checks integer ranges

The following methods lack checks on the following integer arguments, you can see the recommendations above.

Affected source code:

For example, funds deposited in StakedTokenLock could be locked forever with a bad initial configuration.

• StakedTokenLock.sol:20-21

5. Incompatible with tokens without decimals

Cannot set a rewardToken without decimals. Because the pragma used doesn't allow integer underflows, if the reward token used has 0 decimals, an integer underflow will produce a denial of service.

Affected source code:

• LotterySetup.sol:168
• LotterySetup.sol:128

6. The referer should never be yourself

The referrerTicketCount counter of the same player can be increased, in order to increase its rewards in claimPerDraw, for this the msg.sender must be established as referrer.

Affected source code:

• ReferralSystem.sol:60

7. Centralized risk

Nothing ensures that rewards tokens are available to give to the user, so if they are not transferred to the contract, the getReward method will be denied.

Affected source code:

• Staking.sol:98

---

Non critical

8. Unify return criteria

In TicketUtils.sol and LotterySetup.sol files, the methods are sometimes returned with return, other times the return variable is equal, and on other occasions the name of the return variable is not defined, It is recommended to unify the way of writing the code to make it more uniform and readable.

In the same file you can see different behaviors when programming returns. This behavior is observed across multiple contracts.

Affected source code:

• TicketUtils.sol:24
• LotterySetup.sol:152-162

9. Improve naming

Authoritative internals methods must use '_' before the name. We have an example in Ticket.sol:19 where the methods that require authorization and it's internal, a different naming should be used.

Affected source code:

• Ticket.sol:19
• Ticket.sol:23
• RNSourceBase.sol:33
• RNSourceController.sol:38

• Gas
  • 1. Reorder structure layout
  • 2. Optimize conditional
  • 3. Use require instead of assert
  • 4. Use local variable instead of storage

Gas

1. Reorder structure layout

The following structures could be optimized moving the position of certain values in order to save some storage slots:

struct LotterySetupParams {
+   /// @dev Count of numbers user picks for the ticket
+   uint8 selectionSize;
+   /// @dev Max number user can pick
+   uint8 selectionMax;
    /// @dev Token to be used as reward token for the lottery
    IERC20 token;
    /// @dev Parameters of the draw schedule for the lottery
    LotteryDrawSchedule drawSchedule;
    /// @dev Price to pay for playing single game (including fee)
    uint256 ticketPrice;
-   /// @dev Count of numbers user picks for the ticket
-   uint8 selectionSize;
-   /// @dev Max number user can pick
-   uint8 selectionMax;
    /// @dev Expected payout for one ticket, expressed in `rewardToken`
    uint256 expectedPayout;
    /// @dev Array of fixed rewards per each non jackpot win
    uint256[] fixedRewards;
}

Reference:

• https://ethdebug.github.io/solidity-data-representation

Enums are represented by integers; the possibility listed first by 0, the next by 1, and so forth. An enum type just acts like uintN, where N is the smallest legal value large enough to accomodate all the possibilities.

• https://docs.soliditylang.org/en/v0.8.17/internals/layout_in_storage.html#storage-inplace-encoding

Affected source code:

• ILotterySetup.sol:71-73
• ILotterySetup.sol:94

2. Optimize conditional

-       bool notEnoughRetryInvocations = failedSequentialAttempts < maxFailedAttempts;
-       bool notEnoughTimeReachingMaxFailedAttempts = block.timestamp < maxFailedAttemptsReachedAt + maxRequestDelay;
-       if (notEnoughRetryInvocations || notEnoughTimeReachingMaxFailedAttempts) {
+       if ((failedSequentialAttempts < maxFailedAttempts)) || (block.timestamp < maxFailedAttemptsReachedAt + maxRequestDelay)) {
            revert NotEnoughFailedAttempts();
        }

There is an OR conditional that previously computes both conditions. The advantage of an OR conditional is that if one condition is met, the code of the rest of the conditional will not be executed, and therefore gas will be saved, so by modifying the code as shown previously, an optimization will be made when reverting said method.

Affected source code:

• RNSourceController.sol:93-97

Some gas differences:

- [PASS] testSuccessfulSwapSource() (gas: 101837)
+ [PASS] testSuccessfulSwapSource() (gas: 101816)
- [PASS] testSwapSourceFailsWithInsufficientFailedAttempts() (gas: 85632)
+ [PASS] testSwapSourceFailsWithInsufficientFailedAttempts() (gas: 83427)
- [PASS] testSwapSourceFailsWithInsufficientFailedAttemptsWhenNotEnoughTimeSinceReachingMaxFailedAttempts() (gas: 114248)
+ [PASS] testSwapSourceFailsWithInsufficientFailedAttemptsWhenNotEnoughTimeSinceReachingMaxFailedAttempts() (gas: 114226)

3. Use require instead of assert

The assert() and require() functions are a part of the error handling aspect in Solidity. Solidity makes use of state-reverting error handling exceptions. This means all changes made to the contract on that call or any sub-calls are undone if an error is thrown. It also flags an error.

They are quite similar as both check for conditions and if they are not met, would throw an error.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

Affected source code:

• LotterySetup.sol:147

Some gas differences:

- [PASS] testFinalizeInitialPot(uint256,uint256) (runs: 256, μ: 8541365, ~: 8536222)
+ [PASS] testFinalizeInitialPot(uint256,uint256) (runs: 256, μ: 8540713, ~: 8535622)

4. Use local variable instead of storage

Since the same value is previously stored in a local variable, it is cheaper to query its contents than to access the storage for such a query, so the following modification is recommended:

    function finalizeInitialPotRaise() external override {
        if (initialPot > 0) {
            revert JackpotAlreadyInitialized();
        }
        // slither-disable-next-line timestamp
        if (block.timestamp <= initialPotDeadline) {
            revert FinalizingInitialPotBeforeDeadline();
        }
        uint256 raised = rewardToken.balanceOf(address(this));
        if (raised < minInitialPot) {
            revert RaisedInsufficientFunds(raised);
        }
        initialPot = raised;

        // must hold after this call, this will be used as a check that jackpot is initialized
-       assert(initialPot > 0);
+       assert(raised > 0);

        emit InitialPotPeriodFinalized(raised);
    }

Affected source code:

• LotterySetup.sol:147

Some gas differences:

- [PASS] testFinalizeInitialPot(uint256,uint256) (runs: 256, μ: 8541365, ~: 8536222)
+ [PASS] testFinalizeInitialPot(uint256,uint256) (runs: 256, μ: 8541168, ~: 8536222)