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
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:
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.
HardlyDifficult
commented
2 years ago
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.