Nibbl contest - ellahi's results

Low

[L-01] Unsafe ERC20 Operations

Impact

The return value of an external transfer/transferFrom call is not checked

Proof of Concept

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

   /// @notice withdraw ERC20 in the case a held NFT earned ERC20
    function withdrawERC20(address _token) external override {
        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
        IERC20(_token).transfer(msg.sender, IERC20(_token).balanceOf(address(this)));
        emit WithdrawERC20(_token, msg.sender);
    }

    function withdrawMultipleERC20(address[] memory _tokens) external override {
        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
        for (uint256 i = 0; i < _tokens.length; i++) {
            IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));
            emit WithdrawERC20(_tokens[i], msg.sender);
        }
    }

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

function withdrawERC20(address _asset, address _to) external override boughtOut {
        require(msg.sender == bidder, "NibblVault: Only winner");
        IERC20(_asset).transfer(_to, IERC20(_asset).balanceOf(address(this)));
    }

    /// @notice withdraw multiple ERC20s
    /// @param _assets the addresses of assets to be unlocked
    /// @param _to the address where unlocked NFTs will be sent
    function withdrawMultipleERC20(address[] memory _assets, address _to) external override boughtOut {
        require(msg.sender == bidder, "NibblVault: Only winner");
        for (uint256 i = 0; i < _assets.length; i++) {
            IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)));
        }
    }

Recommendation

Use SafeERC20, or ensure that the transfer/transferFrom return value is checked.

Tools used

manual, slither

Gas

[G-01] Cache Array Length Outside of Loop.

Impact

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

Proof of Concept

  Basket.sol::43 => for (uint256 i = 0; i < _tokens.length; i++) {
  Basket.sol::70 => for (uint256 i = 0; i < _tokens.length; i++) {
  Basket.sol::93 => for (uint256 i = 0; i < _tokens.length; i++) {
  NibblVault.sol::506 => for (uint256 i = 0; i < _assetAddresses.length; i++) {
  NibblVault.sol::525 => for (uint256 i = 0; i < _assets.length; i++) {
  NibblVault.sol::547 => for (uint256 i = 0; i < _assets.length; i++) {

Recommendation

Store the array’s length in a variable before the for-loop.

[G-02] ++i costs less gas compared to i++ or i += 1

Impact

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

Proof of Concept

  Basket.sol::43 => for (uint256 i = 0; i < _tokens.length; i++) {
  Basket.sol::70 => for (uint256 i = 0; i < _tokens.length; i++) {
  Basket.sol::93 => for (uint256 i = 0; i < _tokens.length; i++) {
  NibblVault.sol::506 => for (uint256 i = 0; i < _assetAddresses.length; i++) {
  NibblVault.sol::525 => for (uint256 i = 0; i < _assets.length; i++) {
  NibblVault.sol::547 => for (uint256 i = 0; i < _assets.length; i++) {

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.

[G-03] Reduce the size of error messages (Long revert Strings).

Impact

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.

Proof of Concept

  NibblVaultFactory.sol::48 => require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");
  NibblVaultFactory.sol::49 => require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");
  NibblVaultFactory.sol::107 => require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
  NibblVaultFactory.sol::131 => require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
  NibblVaultFactory.sol::141 => require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");
  NibblVaultFactory.sol::149 => require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
  NibblVaultFactory.sol::166 => require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

Recommendation

Shorten the revert strings to fit in 32 bytes, or use custom errors if >0.8.4.

[G-04] Use Custom Errors instead of Revert Strings.

Impact

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

Recommendation

Use custom errors instead of revert strings.

Tools used

c4udit, manual, slither