Nibbl contest - c3phas's results

FINDINGS

Use of solidity's transfer() function might render ETH impossible to withdraw

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when :

1. The claimer smart contracts does not implement a payable function
2. The claimer smart contract does implement a payable function whcih uses more than 2300 gas unit
3. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy ,raising the call's gas usage above 2300

File: Basket.sol line 80

     function withdrawETH(address payable _to) external override {
        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
        _to.transfer(address(this).balance);
        emit WithdrawETH(_to);
    }

Note the line _to.transfer(address(this).balance);

Reccommendation Call() should be used instead of transfer() on an address payable

see consensys blog on transfer

Constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name. Additionally, for complex values, inline comments explaining how they were calculated or why they were chosen are highly recommended. Solidity’s style guide. File: NibblVault.sol line 183

        uint32 _secondaryReserveRatio = uint32((msg.value * SCALE * 1e18) / (_initialTokenSupply * _initialTokenPrice));

File: NibblVault.sol line 195

        uint _primaryReserveBalance = (primaryReserveRatio * _initialTokenSupply * _initialTokenPrice) / (SCALE * 1e18);

File: NibblVault.sol line 226

        secondaryReserveRatio = uint32((secondaryReserveBalance * SCALE * 1e18) / (initialTokenSupply * initialTokenPrice)); //secondaryReserveRatio is updated on every trade 

Lock pragmas to specific compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

File:AccessControlMechanism.sol line 4

pragma solidity ^0.8.0;

Most of the other contracts are using the following pragma directive

pragma solidity ^0.8.10;

Duplicated require()/revert() checks should be refactored to a modifier or function

File: NibblVault.sol line 496

        require(msg.sender == bidder,"NibblVault: Only winner");

The above check has been repeated several times as shown in the following lines

File: NibblVault.sol line 505 File: NibblVault.sol line 516 File: NibblVault.sol line 524 File: NibblVault.sol line 536 File: NibblVault.sol line 546

Other instances File: Basket.sol line 36

        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");

The above check has been repeated in the following

File: Basket.sol line 42 File: Basket.sol line 53 File: Basket.sol line 62 File: Basket.sol line 69 File: Basket.sol line 79 File: Basket.sol line 86 File: Basket.sol line 92

Missing checks for address(0x0) when assigning values to address state variables( Immutable addresses should be 0-checked)

File: NibblVaultFactory.sol line 24

        vaultImplementation = _vaultImplementation;

File: NibblVaultFactory.sol line 25

        feeTo = _feeTo;

File: NibblVaultFactory.sol line 26

        basketImplementation = _basketImplementation;

File: NibblVaultFactory.sol line 100

        pendingBasketImplementation = _newBasketImplementation;

File: NibblVault.sol line 191

        assetAddress = _assetAddress;

File: /ProxyBasket.sol line 20

        implementation = payable(_implementation);

File: ProxyVault.sol line 20

        factory = payable(_factory);
		

File: NibblVault.sol line 487

        curator = _newCurator;

Consider adding zero-address checks in the above.

require(newAddr != address(0));

FINDINGS

Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block

File: NibblVault.sol line 319

                _purchaseReturn = _initialTokenSupply - _totalSupply;

The above line cannot underflow due the check on line 311 which ensures that the operation _initialTokenSupply - _totalSupply would only be performed if _initialTokenSupply is greater than _totalSupply

We can therefore use unchecked blocks as follows

     unchecked { 
			_purchaseReturn = _initialTokenSupply - _totalSupply;
	}

File:NibblVault.sol line 378

                uint256 _tokensPrimaryCurve = _totalSupply - _initialTokenSupply;

The above line cannot underflow due to the check on line 373 which ensures that _totalSupply is greater than _initialTokenSupply

Something similar to my proposal has already been implemented on line 23 as show below

        unchecked {
            _timeElapsed = _blockTimestamp - lastBlockTimeStamp;
        }

Using unchecked blocks to save gas - Increments in for loop can be unchecked

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code

File: NibblVault.sol line 506

    function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {
        require(msg.sender == bidder,"NibblVault: Only winner");
        for (uint256 i = 0; i < _assetAddresses.length; i++) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
        }
    }

The above should be modified to:

    function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {
        require(msg.sender == bidder,"NibblVault: Only winner");
        for (uint256 i = 0; i < _assetAddresses.length; i++) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
							unchecked { i++; }
        }
    }

Other Instances to modify File: NibblVault.sol line 525

        for (uint256 i = 0; i < _assets.length; i++) {

File: Basket.sol line 43

        for (uint256 i = 0; i < _tokens.length; i++) {

File: Basket.sol line 70

        for (uint256 i = 0; i < _tokens.length; i++) {

File: Basket.sol line 93

        for (uint256 i = 0; i < _tokens.length; i++) {

see resource

Cache the length of arrays in loops ~6 gas per iteration

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: NibblVault.sol line 506

    function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {
        require(msg.sender == bidder,"NibblVault: Only winner");
        for (uint256 i = 0; i < _assetAddresses.length; i++) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
        }
    }

The above should be modified to

    function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {
        require(msg.sender == bidder,"NibblVault: Only winner");
				uint256 length = _assetAddresses.length;
        for (uint256 i = 0; i < length; i++) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
        }
    }

Other instances to modify File: NibblVault.sol line 525

        for (uint256 i = 0; i < _assets.length; i++) {

File: Basket.sol line 43

        for (uint256 i = 0; i < _tokens.length; i++) {

File: Basket.sol line 70

        for (uint256 i = 0; i < _tokens.length; i++) {

File: Basket.sol line 93

        for (uint256 i = 0; i < _tokens.length; i++) {

++i costs less gas compared to i++ or i += 1 (~5 gas per iteration)

++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

Instances include:

File: NibblVault.sol line 506

        for (uint256 i = 0; i < _assetAddresses.length; i++) {
           

The above line can be written as follows

        for (uint256 i = 0; i < _assetAddresses.length; ++i) {
           

Other Instances File: NibblVault.sol line 525

        for (uint256 i = 0; i < _assets.length; i++) {

File: Basket.sol line 43

        for (uint256 i = 0; i < _tokens.length; i++) {

File: Basket.sol line 70

        for (uint256 i = 0; i < _tokens.length; i++) {

File: Basket.sol line 93

        for (uint256 i = 0; i < _tokens.length; i++) {

Splitting require() statements that use && saves gas - 8 gas per &&

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).

File:NibblVaultFactory.sol line 107

        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

The above should be modified as follows

        require(basketUpdateTime != 0, "NibblVaultFactory: UPDATE_TIME has not passed");
				require(block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

File:NibblVaultFactory.sol line 131

        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

File:NibblVaultFactory.sol line 149

        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

File:NibblVaultFactory.sol line 166

        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

Proof The following tests were carried out in remix with both optimization turned on and off

	require ( a > 1 && a < 5, "Initialized");
	return  a + 2;
}

Execution cost 21617 with optimization and using && 21976 without optimization and using &&

After splitting the require statement

	require (a > 1 ,"Initialized");
	require (a < 5 , "Initialized");
	return a + 2;
}

Execution cost 21609 with optimization and split require 21968 without optimization and using split require

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

consequences:

• Each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)

• Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

File:AccessControlMechanism.sol line 12

    bytes32 public constant FEE_ROLE = keccak256("FEE_ROLE");

File:AccessControlMechanism.sol line 13

    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

File:AccessControlMechanism.sol line 14

    bytes32 public constant IMPLEMENTER_ROLE = keccak256("IMPLEMENTER_ROLE");

File: NibblVault.sol line 51

    bytes32 private constant PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");

File: EIP712Base.sol line 7

    bytes32 internal constant EIP712_DOMAIN_TYPEHASH = keccak256( bytes("EIP712Domain(string name,string version,uint256chainId,address verifyingContract)"));

Use shorter revert strings(less than 32 bytes)

You can (and should) attach error reason strings along with require statements to make it easier to understand why a contract call reverted. These strings, however, take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

File:NibblVaultFactory.sol line 48

        require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");

File:NibblVaultFactory.sol line 49

        require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");

Other instances to modify File:NibblVaultFactory.sol line 107

        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

File:NibblVaultFactory.sol line 131

        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

File:NibblVaultFactory.sol line 141

        require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");

File:NibblVaultFactory.sol line 149

        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

File:NibblVaultFactory.sol line 166

        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors.

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)

see Source

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