Wenwin contest - Madalad's results

The next generation of chance-based gaming.

General Information

Platform: Code4rena

Start Date: 06/03/2023

Pot Size: $36,500 USDC

Total HM: 8

Participants: 93

Period: 3 days

Judge: cccz

Total Solo HM: 3

Id: 218

League: ETH

Wenwin

Findings Distribution

Researcher Performance

Rank: 34/93

Findings: 2

Award: $103.11

QA:

grade-b

Gas:

grade-a

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Wenwin

Findings Information

🌟 Selected for report: adriro

Also found by: 0x1f8b, 0xAgro, 0xSmartContract, 0xfuje, 0xkazim, 0xnev, Aymen0909, Bason, Cyfrin, DadeKuma, LethL, Madalad, MohammedRizwan, Rolezn, SAAJ, SunSec, Udsen, Yukti_Chinta, ast3ros, bin2chen, brgltd, bshramin, btk, bugradar, catellatech, cryptostellar5, descharre, dontonka, erictee, fatherOfBlocks, georgits, glcanvas, hl_, horsefacts, igingu, juancito, lukris02, martin, nadin, nomoi, peanuts, pipoca, sakshamguruji, seeu, slvDev, tnevler, zaskoh

Awards

21.7018 USDC - $21.70

Labels

bug

grade-b

QA (Quality Assurance)

sponsor confirmed

edited-by-warden

Q-54

External Links

Link to GitHub issue

Non-Critical Summary

	Issue	Instances
[N-01]	Include all relevant parameters in events	3
[N-02]	`finalizeInitialPotRaise` is vulnerable to frontrunning	1
[N-03]	Missing argument check in `swapSource`	1
[N-04]	Use fixed compiler version	2
[N-05]	Implementing `renounceOwnership` is dangerous	3
[N-06]	Remove unused imports	5
[N-07]	Missing `address(0)` checks in constructor/initialize	3

Total issues: 7 Total instances: 18

Non-Critical Issues

[N-01] Include all relevant parameters in events

Include an oldSource parameter in RNSourceController's SourceSet event to provide full information about the change:

[N-02] `finalizeInitialPotRaise` is vulnerable to frontrunning

Should the contract meet the minInitialPot threshold before the team has finished funding the contract completely or before they are ready for the lottery to begin, anyone can call the finalizeInitialPotRaise function to begin the lottery.

Make sure to either fully fund the contract in one transaction, or add access control to finalizeInitialPotRaise.

Instances: 1

src/LotterySetup.sol#L132

[N-03] Missing argument check in `swapSource`

A check should be added to swapSource to ensure that newSource is not equal to the old source value.

src/RNSourceController.sol#L89

[N-04] Use fixed compiler version

A floating pragma risks a different compiler version being used in production vs testing, which poses security risks.

Instances: 2

[N-05] Implementing `renounceOwnership` is dangerous

Typically, the contract's owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities.

The OpenZeppelin's Ownable used in this project contract implements renounceOwnership. This can represent a certain risk if the ownership is renounced for any other reason than by design.

Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.

It is recommended to either reimplement the function to disable it, or to clearly specify if it is part of the contract design.

Instances: 3

[N-06] Remove unused imports

Improves readability and saves gas.

Instances: 5

[N-07] Missing `address(0)` checks in constructor/initialize

Failing to check for invalid parameters on deployment may result in an erroneous input and require an expensive redeployment.

Instances: 3

#0 - thereksfour
2023-03-12T13:09:34Z

6 INFO

#1 - c4-judge
2023-03-12T13:09:37Z

thereksfour marked the issue as grade-b

#2 - c4-sponsor
2023-03-13T12:05:42Z

TutaRicky marked the issue as sponsor confirmed

Findings Information

🌟 Selected for report: Rolezn

Also found by: 0x1f8b, 0x6980, 0xSmartContract, 0xhacksmithh, 0xnev, Haipls, Inspectah, JCN, LethL, Madalad, MiniGlome, Pheonix, Rageur, RaymondFam, ReyAdmirado, SAAJ, Sathish9098, adriro, air, arialblack14, atharvasama, c3phas, ch0bu, ddimitrov22, descharre, hunter_w3b, igingu, matrix_0wl, rokso, saneryee, schrodinger, slvDev, volodya, yongskiws

Awards

81.411 USDC - $81.41

Labels

bug

G (Gas Optimization)

grade-a

sponsor confirmed

edited-by-warden

G-34

External Links

Link to GitHub issue

Gas Optimizations Summary

	Issue	Instances
[G-01]	Functions guaranteed to revert when called by normal users can be marked `payable`	4
[G-02]	Use assembly to check for `address(0)`	10
[G-03]	Cache storage variables rather than re-reading from storage	5
[G-04]	Inline `internal` functions that are only called once	7
[G-05]	Use `unchecked` for operations that cannot overflow/underflow	11
[G-06]	Pack state variables into fewer storage slots	1
[G-07]	Use `private` rather than `public` for constants	9
[G-08]	Change `public` functions to `external`	2
[G-09]	Use a more recent version of Solidity	2
[G-10]	Naming a return variable and still calling the `return` keyword wastes gas	11
[G-11]	`x += y` costs more gas than `x = x + y` for state variables	3
[G-12]	Usage of `uint` smaller than 32 bytes (256 bits) incurs overhead	17
[G-13]	`++i` costs less gas than `i++`	7

Total issues: 13 Total instances: 89

Gas Optimizations

[G-01] Functions guaranteed to revert when called by normal users can be marked `payable`

If a function modifier such as onlyOwner 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 (2400 per instance).

Instances: 4

[G-02] Use assembly to check for `address(0)`

Saves 16000 deployment gas per instance and 6 runtime gas per instance.

assembly {
 if iszero(_addr) {
  mstore(0x00, "zero address")
  revert(0x00, 0x20)
 }
}

Instances: 10

[G-03] Cache storage variables rather than re-reading from storage

Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read.

Caching a mapping’s value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations. Caching an array’s struct avoids recalculating the array offsets into memory/calldata.

Instances: 5

[G-04] Inline `internal` functions that are only called once

Saves 20-40 gas per instance. See https://blog.soliditylang.org/2021/03/02/saving-gas-with-simple-inliner/ for more details.

Instances: 7

[G-05] Use `unchecked` for operations that cannot overflow/underflow

By bypassing Solidity's built in overflow/underflow checks using unchecked, we can save gas. This is especially beneficial for the index variable within for loops (saves 120 gas per iteration).

Instances: 11

[G-06] Pack state variables into fewer storage slots

If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper.

For more information about variable packing, see here.

Instances: 1

src/RNSourceController.sol#L10

[G-07] Use `private` rather than `public` for constants

Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table. If needed to be viewed externally, the values can be read from the verified contract source code.

Instances: 9

[G-08] Change `public` functions to `external`

Functions marked as public that are not called internally should be set to external to save gas and improve code quality. External call cost is less expensive than of public functions.

Instances: 2

[G-09] Use a more recent version of Solidity

Use a Solidity version of at least 0.8.2 to get simple compiler automatic inlining.

Use a Solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads.

Use a Solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings.

Use a Solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value.

Use a Solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>).

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions.

Instances: 2

[G-10] Naming a return variable and still calling the `return` keyword wastes gas

Instances: 11

[G-11] `x += y` costs more gas than `x = x + y` for state variables

Instances: 3

[G-12] Usage of `uint` smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

Consider using a larger size then downcasting where needed.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html

Instances: 17

[G-13] `++i` costs less gas than `i++`

True for --i/i-- as well, and is especially important in for loops. Saves 5 gas per iteration.

Instances: 7

#0 - c4-judge
2023-03-12T14:42:08Z

thereksfour marked the issue as grade-a

#1 - c4-sponsor
2023-03-14T13:18:54Z

TutaRicky marked the issue as sponsor confirmed

Wenwin contest - Madalad's results

The next generation of chance-based gaming.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-54] QA Report - $21.70

Findings Information

Awards

Labels

External Links

Non-Critical Summary

Non-Critical Issues

[N-01] Include all relevant parameters in events

[N-02] finalizeInitialPotRaise is vulnerable to frontrunning

[N-03] Missing argument check in swapSource

[N-04] Use fixed compiler version

[N-05] Implementing renounceOwnership is dangerous

[N-06] Remove unused imports

[N-07] Missing address(0) checks in constructor/initialize

#0 - thereksfour2023-03-12T13:09:34Z

#1 - c4-judge2023-03-12T13:09:37Z

#2 - c4-sponsor2023-03-13T12:05:42Z

[G-34] Gas Report - $81.41

Findings Information

Awards

Labels

External Links

Gas Optimizations Summary

Gas Optimizations

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

[G-02] Use assembly to check for address(0)

[G-03] Cache storage variables rather than re-reading from storage

[G-04] Inline internal functions that are only called once

[G-05] Use unchecked for operations that cannot overflow/underflow

[G-06] Pack state variables into fewer storage slots

[G-07] Use private rather than public for constants

[G-08] Change public functions to external

[G-09] Use a more recent version of Solidity

[G-10] Naming a return variable and still calling the return keyword wastes gas

[G-11] x += y costs more gas than x = x + y for state variables

[G-12] Usage of uint smaller than 32 bytes (256 bits) incurs overhead

[G-13] ++i costs less gas than i++

#0 - c4-judge2023-03-12T14:42:08Z

#1 - c4-sponsor2023-03-14T13:18:54Z

[N-02] `finalizeInitialPotRaise` is vulnerable to frontrunning

[N-03] Missing argument check in `swapSource`

[N-05] Implementing `renounceOwnership` is dangerous

[N-07] Missing `address(0)` checks in constructor/initialize

#0 - thereksfour
2023-03-12T13:09:34Z

#1 - c4-judge
2023-03-12T13:09:37Z

#2 - c4-sponsor
2023-03-13T12:05:42Z

[G-01] Functions guaranteed to revert when called by normal users can be marked `payable`

[G-02] Use assembly to check for `address(0)`

[G-04] Inline `internal` functions that are only called once

[G-05] Use `unchecked` for operations that cannot overflow/underflow

[G-07] Use `private` rather than `public` for constants

[G-08] Change `public` functions to `external`

[G-10] Naming a return variable and still calling the `return` keyword wastes gas

[G-11] `x += y` costs more gas than `x = x + y` for state variables

[G-12] Usage of `uint` smaller than 32 bytes (256 bits) incurs overhead

[G-13] `++i` costs less gas than `i++`

#0 - c4-judge
2023-03-12T14:42:08Z

#1 - c4-sponsor
2023-03-14T13:18:54Z