Low

1. Mixing and Outdated compiler

The pragma version used are:

pragma solidity >=0.8.0;
pragma solidity ^0.8.0;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.3:

Optimizer: Fix bug on incorrect caching of Keccak-256 hashes.

0.8.4:

ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected.

0.8.9:

Immutables: Properly perform sign extension on signed immutables.
User Defined Value Type: Fix storage layout of user defined value types for underlying types shorter than 32 bytes.

0.8.13:

Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

2. Lack of checks

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 for address(0):

3. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because of human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

4. Avoid using `tx.origin`

tx.origin is a global variable in Solidity that returns the address of the account that sent the transaction.

Using the variable could make a contract vulnerable if an authorized account calls a malicious contract. You can impersonate a user using a third party contract.

This can make it easier to create a vault on behalf of another user with an external administrator (by receiving it as an argument).

Affected source code:

5. Unsafe math

A user supplied argument can be passed as zero to multiplication operation.

Reference:

https://slowmist.medium.com/analysis-of-the-treasuredao-zero-fee-exploit-73791f4b9c14

Affected source code:

6. 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.

getGobblerData[gobblerId].emissionMultiple = uint32(burnedMultipleTotal * 2);

Affected source code:

Non critical

7. Avoid hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code.

Affected source code:

Use the selector instead of the raw value:

8. Use tokens with ERC20 permit

The erc20 signature approval extension is a major usability enhancement that helps users and projects reap its benefits in the future.

The use of this feature is fully recommended.

Reference:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/draft-ERC20Permit.sol

Affected source code:

Goo.sol:58

9. Avoid hardcoded values

Affected source code:

Use the selector instead of the raw value:

ERC721.sol:148-150

code-423n4 / 2022-09-artgobblers-findings

QA Report #271

Low

1. Mixing and Outdated compiler

2. Lack of checks

3. Lack of ACK during owner change

4. Avoid using `tx.origin`

5. Unsafe math

6. Integer overflow by unsafe casting

Non critical

7. Avoid hardcoded values

8. Use tokens with ERC20 permit

9. Avoid hardcoded values

1. Mixing and Outdated compiler

2. Lack of checks

3. Lack of ACK during owner change

4. Avoid using tx.origin

5. Unsafe math

6. Integer overflow by unsafe casting

7. Avoid hardcoded values

8. Permit