More Known Issues

[PatrickAlphaC]

Report

All Issues

	Issue	Instances
M-1	Centralization Risk for trusted owners	4

	Issue	Instances
L-1	abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()	1

	Issue	Instances
NC-1	Typos	60

	Issue	Instances
GAS-1	Using bools for storage incurs overhead	1
GAS-2	Cache array length outside of loop	1
GAS-3	++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)	1
GAS-4	Using private rather than public for constants, saves gas	2

Gas Optimizations

	Issue	Instances
GAS-1	Using bools for storage incurs overhead	1
GAS-2	Cache array length outside of loop	1
GAS-3	++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)	1
GAS-4	Using private rather than public for constants, saves gas	2

[GAS-1] Using bools for storage incurs overhead

Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past. See source.

Instances (1):

File: src/ProxyFactory.sol

71:     mapping(address => bool) public whitelistedTokens;

[GAS-2] Cache array length outside of loop

If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).

Instances (1):

File: src/ProxyFactory.sol

83:         for (uint256 i; i < _whitelistedTokens.length;) {

[GAS-3] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

Instances (1):

File: src/ProxyFactory.sol

87:                 i++;

[GAS-4] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public 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

Instances (2):

File: src/ProxyFactory.sol

64:     uint256 public constant EXPIRATION_TIME = 7 days;

65:     uint256 public constant MAX_CONTEST_PERIOD = 28 days;

Non Critical Issues

	Issue	Instances
NC-1	Typos	60

[NC-1] Typos

Instances (60):

File: src/Distributor.sol

- 3: // Layout of Contract:
+ 3: // Layout of Contract: version, imports, errors, interfaces, libraries, contracts, type declarations, state variables, events, modifiers, functions

- 4: // version
+ 4: // Layout of Functions: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private, view & pure functions

- 5: // imports
+ 5: //////////////////////

- 6: // errors
+ 6: /////// Error ////////

- 7: // interfaces, libraries, contracts
+ 7: //////////////////////

- 8: // Type declarations
+ 8: //////////////////////////////////////

- 9: // State variables
+ 9: /////// Immutable Variables //////////

- 10: // Events
+ 10: //////////////////////////////////////

- 11: // Modifiers
+ 11: uint8 private constant VERSION = 1; // version is 1 for now

- 12: // Functions
+ 12: uint256 private constant COMMISSION_FEE = 500; // this can be changed in the future

- 14: // Layout of Functions:
+ 14: // a constant value of 10,000 (basis points) = 100%

- 15: // constructor
+ 15: // prize distribution event. data is for logging purpose

- 16: // receive function (if exists)
+ 16: ////////////////////////////

- 17: // fallback function (if exists)
+ 17: /////// Constructor ////////

- 18: // external
+ 18: ////////////////////////////

- 19: // public
+ 19: /// @dev initiate the contract with factory address and other key addresses, fee rate

- 20: // internal
+ 20:     // uint256 version, // for future use

- 21: // private
+ 21:     FACTORY_ADDRESS = factoryAddress; // initialize with deployed factory address beforehand

- 22: // view & pure functions
+ 22:     STADIUM_ADDRESS = stadiumAddress; // official address to receive commission fee

- 40:     //////////////////////
+ 40: ////////////////////////////////////////////

- 41:     /////// Error ////////
+ 41: /////// External & Public functions ////////

- 42:     //////////////////////
+ 42: ////////////////////////////////////////////

- 52:     //////////////////////////////////////
+ 52: ////////////////////////////////////////////

- 53:     /////// Immutable Variables //////////
+ 53: /////// Internal & Private functions ///////

- 54:     //////////////////////////////////////
+ 54: ////////////////////////////////////////////

- 56:     uint8 private constant VERSION = 1; // version is 1 for now
+ 56:     // token address input check

- 59:     uint256 private constant COMMISSION_FEE = 500; // this can be changed in the future
+ 59:     // winners and percentages input check

- 60:     // a constant value of 10,000 (basis points) = 100%
+ 60:     // check if totalPercentage is correct

- 63:     // prize distribution event. data is for logging purpose
+ 63:     // if there is no token to distribute, then revert

- 66:     ////////////////////////////
+ 66:     uint256 winnersLength = winners.length; // cache length

- 67:     /////// Constructor ////////
+ 67:     // send commission fee as well as all the remaining tokens to STADIUM_ADDRESS to avoid dust remaining

- 68:     ////////////////////////////
+ 68: ///////////////////////////////////////////

- 69:     /// @dev initiate the contract with factory address and other key addresses, fee rate
+ 69: /////// Getter pure/view functions ////////

- 71:         // uint256 version, // for future use
+ 71: ///////////////////////////////////////////

78:         FACTORY_ADDRESS = factoryAddress; // initialize with deployed factory address beforehand

79:         STADIUM_ADDRESS = stadiumAddress; // official address to receive commission fee

82:     ////////////////////////////////////////////

83:     /////// External & Public functions ////////

84:     ////////////////////////////////////////////

101:     ////////////////////////////////////////////

102:     /////// Internal & Private functions ///////

103:     ////////////////////////////////////////////

119:         // token address input check

124:         // winners and percentages input check

134:         // check if totalPercentage is correct

141:         // if there is no token to distribute, then revert

144:         uint256 winnersLength = winners.length; // cache length

153:         // send commission fee as well as all the remaining tokens to STADIUM_ADDRESS to avoid dust remaining

176:     ///////////////////////////////////////////

177:     /////// Getter pure/view functions ////////

178:     ///////////////////////////////////////////

File: src/Proxy.sol

- 3: // Layout of Contract:
+ 3: // Layout of a Contract:

- 14: // Layout of Functions:
+ 14: // Layout of a Function:

File: src/ProxyFactory.sol

- 68:     /// @dev The contest doesn't exist when value is 0
+ 68:     /// @dev The contest doesn't exist when the value is 0

- 70:     /// @dev record whitelisted tokens
+ 70:     /// @dev Record whitelisted tokens

- 133:         // can set close time to current time and end it immediately if organizer wish
+ 133:         // Can set close time to current time and end it immediately if organizer wish

- 215:         // distribute only when it exists and expired
+ 215:         // Distribute only when it exists and expired

- 221:     /// @dev Calculate the proxy address using salt and implementation address
+ 221:     /// @dev Calculate the proxy address using the salt and implementation address

- 246:     /// @dev the data passed in should be the calling data of the distributing logic
+ 246:     /// @dev The data passed in should be the calling data of the distributing logic

- 255:     /// @dev Calculate salt using contest organizer address and contestId, implementation address
+ 255:     /// @dev Calculate salt using contest organizer address and contestId, and implementation address

Low Issues

	Issue	Instances
L-1	abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()	1

[L-1] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead

Instances (1):

File: src/ProxyFactory.sol

227:         bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, keccak256(code)));

Medium Issues

	Issue	Instances
M-1	Centralization Risk for trusted owners	4

[M-1] Centralization Risk for trusted owners

Impact:

Contracts have owners with privileged rights to perform admin tasks and need to be trusted to not perform malicious updates or drain funds.

Instances (4):

File: src/ProxyFactory.sol

37: contract ProxyFactory is Ownable, EIP712 {

81:     constructor(address[] memory _whitelistedTokens) EIP712("ProxyFactory", "1") Ownable() {

184:     ) public onlyOwner returns (address) {

211:     ) public onlyOwner {