Nibbl contest - Chom's results

Minus before addition

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

_buyoutBid = msg.value + (primaryReserveBalance - fictitiousPrimaryReserveBalance) + secondaryReserveBalance;

This line perform minus before addition which risk underflow. Should add before minus in all case. Change code to this

_buyoutBid = msg.value + primaryReserveBalance + secondaryReserveBalance - fictitiousPrimaryReserveBalance;

This add before minus, so never underflow.

Use "" for data instead of "0"

"0" is not 0x "" is 0x

Sending empty data should send "" not "0"

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

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

IERC1155(_asset).safeTransferFrom(address(this), _to, _assetID, balance, "0");

should be

IERC1155(_asset).safeTransferFrom(address(this), _to, _assetID, balance, "");

Caching the length in for loops

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

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)

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

        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");
        }

Can be optimized to

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

The increment in for loop postcondition can be made unchecked

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant!

Apply this to all part in your code with for loops. For example

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

        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");
        }

Can be optimized to

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

            unchecked { i++; }
        }

Consider using custom errors instead of revert strings

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deployment cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Any require statement in your code can be replaced with custom error for example:

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

Can be replaced with

// declare error before contract declaration
error OnlyWinner();

if (msg.sender != bidder) revert OnlyWinner();