Nibbl contest - pashov's results

Result of ERC20.transfer method is ignored

Proof of concept

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L85-L97

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L515-L528

Basket#withdrawERC20, Basket#Basket.withdrawMultipleERC20, NibblVault#withdrawERC20, NibblVault#Basket.withdrawMultipleERC20 all call IERC20 transfer method without checking for its return value (bool success). This is a problem because some ERC20 tokens do not revert on error but return “false” instead.

Impact

This can lead to some confusion on the ERC20 withdrawer side and to some wasted gas due to need to call withdraw methods again.

Recommendation

Always use SafeERC20.safeTransfer instead of ERC20.transfer so you handle ERC20 tokens that do not revert on error

Using address payable’s transfer method is discouraged

Proof of concept

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L80

Basket.sol line 80 sends ETH by using the low-level transfer call which is discouraged. Transfer can take up to a maximum of 2300 gas, which can lead to inability to withdraw the ether if the recipient is a smart contract with a receive function that uses more than 200 gas

Impact

This might lead to stuck ether for a user of the protocol. This might be a Medium risk finding.

Recommendation

Change “_to**.transfer**(address(this).balance);” to “_to**.call{value:** address(this).balance**}**("");”

Missing non-zero address checks

Proof of concept

(not putting Github links because they are ~10 here)

Externally provided function arguments for address parameters can mistakenly be set to the zero address

function parameters of type address that do not have a non-zero address check:

NibblVault#initialize - _assetAddress, _curator

NibblVault#updateCurator - _newCurator

NibblVaultFactory#constructor - _vaultImplementation, _feeTo, _basketImplementation

NibblVaultFactory#proposeNewBasketImplementation - _newBasketImplementation

NibblVaultFactory#proposeNewAdminFeeAddress - _newFeeAddress

NibblVaultFactory#proposeNewVaultImplementation - _newVaultImplementation

ProxyBasket#constructor - _implementation

ProxyVault#constructor - _factory

Impact

Mistakenly setting an address type field to the zero address can result in a need to send a new correct transaction (wasted gas) in the context of setter functions, and in a need for contract redeployment in the context of constructors or initialiser functions.

Recommendation

Add a require(_functionParam ! = address(0)) check for all places mentioned.

Contracts are using 0.8.x Solidity version but do not use Custom Errors

Proof of concept

Using Solidity custom errors is gas efficient and can be helpful for external integrations’ error handling. Having a require statement with an error string is costly both for deployment cost for contract deployer and failed transaction cost for the user

Impact

This can save users a good amount of gas on failed transactions.

Recommendation

Replace all require statements in the code with a revert CustomError statements

Contract functions that are only called externally should be declared external

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L64-L93

Proof of concept

All contract functions in NibblVaultFactory.sol that are declared as public can be declared as external.

Impact

Functions that are declared as external have their arguments in calldata instead of memory which saves gas for users

Recommendation

Change all public methods in NibblVaultFactory.sol to external

Inefficient usage of for loops

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L43

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L70

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L43

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L506

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L525

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L547

Proof of concept

Basket.sol and NibblVault.sol contain for loops that are implemented inefficiently in terms of gas for the EVM. For example omitting assigning a default-zero type variable to zero, caching array's length, using ++i instead of i++ can save a good chunk of gas, especially if the loop is long running.

Impact

Gas savings for protocol users

Recommendation

1. Don't initialise index counter variable to zero - zero is its default value and reinitialising to zero costs extra gas
2. When the for loop end check expression is checking for some array's length, always cache the length of the array in a separate stack variable.
3. Use ++i instead of i++ in for loops (small gas saving)
4. You can wrap the ++i in an unchecked block since you have a for loop end check expression anyway

Example for loop implemented using the recommendations:

for (uint256 i = 0; i < _assets.length; i++) {
            uint256 balance = IERC1155(_assets[i]).balanceOf(address(this),  _assetIDs[i]);
            IERC1155(_assets[i]).safeTransferFrom(address(this), _to, _assetIDs[i], balance, "0");
 }

With recommended gas optimisations:

uint256 assetsLength = _assets.length;
for (uint256 i; i < assetsLength;) {
	     uint256 balance = IERC1155(_assets[i]).balanceOf(address(this),  _assetIDs[i]);
        IERC1155(_assets[i]).safeTransferFrom(address(this), _to, _assetIDs[i], balance, "0");

    unchecked {
        ++i;
    }
}