Nibbl contest - cloudjunky's results

Inconsistent Pragma Definitions and floating pragma

Issue

A fixed Solidity pragma of 0.8.10 is used throughout the project but there are examples where different versions and floating pragma are used. For example;

• ^0.8.0 in IAccessControlMechanism.sol, AccessControlMechanism.sol

Remediation

All contracts should use the same fixed Solidity version (i.e. not floating) across the code based. Floating pragma is noted as a smart contract weakness in SWC-103.

ETH transfer uses .transfer() and the return value is not checked

Issue

The withdrawETH() Basket.sol uses the transfer() function with can cause a number of issues in in production use. For example;

This can lead to the locking of funds as;

1. The transfer() requires the recipient to have a payable callback and may result in the locking of funds.
2. It only provides 2300 units of gas which is only enough to emit something and may fail of the destination address is a contract or a contract called through a proxy.
3. Using .call() allows gas to be set at a higher value than the default for .transfer() which is 2300 unites.

Remediation

The .transfer() method should be changed to use .call(). For example, withdrawETH() in Basket.sol` should be changed from this;

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

To this;

function withdrawETH(address payable _to) external override {
  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
  (bool success, ) = payable(_to).call{value: address(this).balance}("");
  require(success, "Basket: ETH transfer failed");
  emit WithdrawETH(_to);
}

Note in the example above that a boolean success value is initialised by the .call{} and is checked to ensure that the transfer has been successful or reverts.

Incomplete or inconsistent natspec

Issue

There's missing natspec documentation on a number of functions and important fields like @param and @return are sometimes missing. For example;

• _sellPrimaryCurve()inNibblVault.sol has documentation for @param _amount but not _totalSupply.
• _sellSecondaryCurve() in NibblVault.sol has documentation for @param _amount but not _totalSupply.
• withdrawERC721() , withdrawERC721Unsafe() and withdrawERC1155() in Basket.sol have documentation for @param _token and _tokenId but not _to.

Remediation

Proper natspec documentation with @param and @return populated would significantly improve the readability of the code base and ensure reviewers aren't guessing as to the intent of arguments to functions. A good standard is to full document all public and external functions with @param and @return so that users of the contract can understand the functionality better.