Nibbl contest - 0x1f8b's results

Low

Obsolete pragma

The pragma version used is:

pragma solidity 0.8.10;
pragma solidity ^0.8.0;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

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

The minimum required version should be 0.8.14

Lack of empty address checks

The following methods have a lack 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).

address(0):

• NibblVaultFactory.sol#L26
• AccessControlMechanism.sol#L19

AccessControl / Pausable

The contract NibblVaultFactory is AccessControl and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while paused should be avoided.

Affected source code:

• NibblVaultFactory.sol#L17

Use encode instead of encodePacked for hashig

Use of abi.encodePacked in NibblVaultFactory is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

There is also discussion of removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• NibblVaultFactory.sol#L50
• NibblVaultFactory.sol#L70-L71
• NibblVaultFactory.sol#L81
• NibblVaultFactory.sol#L89-L91

Unsafe ERC20 calls

The following code doesn't check the result of the ERC20 calls. ERC20 standard specify that the token can return false if these calls fails, so it's mandatory to check the result of these ERC20 methods.

Reference:

• eip-20

NOTES: The following specifications use syntax from Solidity 0.4.17 (or above) Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

Affected source code for transfer:

• Basket.sol#L87
• Basket.sol#L94
• NibblVault.sol#L517
• NibblVault.sol#L526

Lack of ACK during owner change

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

Because an 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:

• NibblVault.sol#L485

Non critical

Outdated packages

The packages used are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts": "^4.5.0",
"@openzeppelin/contracts-upgradeable": "^4.5.0",

Affected source code:

• package.json#L28-L29

Use call to transfer ether

Because to transfer ether the .transfer method (which is capped at 2300 gas) is used instead of .call which is limited to the gas provided by the user. If a contract that has a fallback method more expensive than 2300 gas, it will be impossible for a contract receive funds from Basket contract.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected source code:

• Basket.sol#L80

Use interval for update window

If a maximum of time is not used during the update proposal, it is possible that the update will be made at the beginning or during deploy it, and after a few years, the change will be accepted, and users won't be aware of that. It is convenient to use a maximum expiration time of the proposal.

Affected source code:

• NibblVaultFactory.sol#L107
• NibblVaultFactory.sol#L131
• NibblVaultFactory.sol#L149
• NibblVaultFactory.sol#L166

Possible loss of token 0

Token 0 is more or less the owner of the Basket contract. If this token is transfered to the wrong address, for example address(this), this ownership could be losed.

It would be convenient to block the transfer when the token is 0 and to is address(this) (address(0) it's already checked.).

Affected source code:

• Basket.sol#L37-L54

Use abstract for base contracts

Abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• Twav.sol#L4
• EIP712Base.sol#L5
• AccessControlMechanism.sol#L8

Wrong initialization

Wrong token name is used during the initialization of NibblVault.INIT_EIP712. NibblVault is used instead of _tokenName.

Affected source code:

• NibblVault.sol#L186

Lack of lock in sell method

if buy have lock, sell should have it, because the danger is the same.

Affected source code:

• NibblVault.sol#L362

Use constants

Use constant instead of storage for:

• UPDATE_TIME in NibblVaultFactoryData.sol#L6

Reduce math operations storing the value in a constant:

2**32:

• NibblVault.sol#L303
• NibblVault.sol#L365

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

If it's not possible to use error codes due to the pragma used, it is recommended to reduce the strings to less than 32 bytes.

Affected source code:

• NibblVaultFactory.sol#L48-L49
• NibblVaultFactory.sol#L107

Avoid redundant code

Two methods are exposed in the abi for the same value.

getTwavObservations returns the same value as public twavObservations:

• Twav.sol#L44

Remove _defaultAdminRole var and use DEFAULT_ADMIN_ROLE:

• AccessControlMechanism.sol#L20

There are no sense to have an unsafe version of withdrawERC721Unsafe:

• Basket.sol#L49-L56

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

It is also recommended to not initialize the counter variable and surround the increment with an unchecked region.

Affected source code:

• Basket.sol#L43
• Basket.sol#L70
• Basket.sol#L93
• NibblVault.sol#L506
• NibblVault.sol#L525
• NibblVault.sol#L547

There's no need to set default values for variables.

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• Basket.sol#L43
• Basket.sol#L70
• Basket.sol#L93

Use the right type

Move unlocked close to an address type and change the value to be a boolean to share the same storage slot:

• NibblVault.sol#L126