Nibbl contest - TerrierLover's results

[QA-1] Inconsistent naming - use capital letters with underscores for constant

As solidity document mentions here, it should be named with all capital letters with underscores separating words. However, following state variable does not follow this pattern.

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L28

---

[QA-2] Use uint256 instead of uint for the consistency

Throughout the codebase, uint256 is used. However, the following variables in NibblVault.sol do not follow this pattern.

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L195

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L289

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L336

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L348

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L455

---

[QA-3] Use OpenZeppelin's upgradeable files

NibblVault.sol uses ERC20Upgradeable, but the interfaces used in this contract does not use OpenZeppelin's upgradeable files.

• IERC20: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol
• IERC721: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/IERC721.sol
• IERC1155: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/IERC1155.sol

---

[QA-4] Incorrect indentations

Throughout the codebase, there are 4 spaces in one indentation. However, following 3 codes do not follow this pattern.

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L238

    function _chargeFeeSecondaryCurve(uint256 _amount) private returns(uint256) {
       address payable _factory = factory;
        uint256 _adminFeeAmt = NibblVaultFactory(_factory).feeAdmin();

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L253

    function getMaxSecondaryCurveBalance() private view returns(uint256){
            return ((secondaryReserveRatio * initialTokenSupply * initialTokenPrice) / (1e18 * SCALE));
    }

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L266

    function getCurrentValuation() private view returns(uint256) {
            return totalSupply() < initialTokenSupply ? (secondaryReserveBalance * SCALE /secondaryReserveRatio) : ((primaryReserveBalance) * SCALE  / primaryReserveRatio);
    }

---

[QA-5] Typo: Shouljd use initialize instead of initialise

Basket.sol has initialise function while NibblVault.sol has initialize function.

OZ's example uses initialize so Basket.sol also should use initialize. https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializers

---

[QA-6] Better adding an error message at require

Throughout the codebase, require check sets error messages. However, findPositionInMaxExpArray function does not follow this pattern, and it simply has require(false). It is worth adding why this part should always fail.

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L356

---

[QA-7] Wrong comments at buy fuction and sell function

Comments used at buy fuction and sell function in NibblVault.sol seem to be wrong.

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L297

    /// @dev if current totalSupply < initialTokenSupply AND _amount to buy tokens for is greater than (maxSecondaryCurveBalance - currentSecondaryCurveBalance) then buy happens on secondary curve and primary curve both

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L358

    /// @dev if totalSupply > initialTokenSupply AND _amount to sell is greater than (_amtIn > totalSupply - initialTokenSupply) then sell happens on primary curve and secondary curve both

They say buy happens on secondary curve and primary curve both and sell happens on secondary curve and primary curve both, but buy and sell only happen at primary curve only.

---

[QA-8] Wrong comment at initiateBuyout function

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L395

/// @dev bidder needs to send funds equal to current valuation - ((primaryReserveBalance - fictitiousPrimaryReserveBalance) + secondaryReserveBalance) to initiate buyout

It seems that current valuation - ((primaryReserveBalance - fictitiousPrimaryReserveBalance) + secondaryReserveBalance) is no longer used at initiateBuyout function, and this comment seems to be wrong.

[GAS-1] Use != 0 instead of > 0 on uint variables

uint variables will never be lower than 0. Therefore, > 0 and != 0 have same meanings. Using != 0 can reduce the gas deployment cost, so it is worth using != 0 wherever possible.

Here are list of the gas improvements by using != 0.

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L227

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L243

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L188

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L296

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L323

---

[GAS-2] No need to set 0 on uint variables

The default value of uint varibles are 0. Therefore, there is no need to set 0 on uint variables. Not setting 0 on uint variables can reduce the deployment gas cost.

Here are list of the gas improvements.

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

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L285

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L312

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L369

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L419

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L460

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

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

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

---

[GAS-3] Usage of unchecked

Following variables or operations can be wrapped by unchecked to reduce the gas cost.

[1] --i used in the for loop when the end condition is uint or constant

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L296

for (uint8 i = MAX_PRECISION; i > 0;) {
    x = (x * x) / FIXED_1; // now 1 < x < 4
    if (x >= FIXED_2) {
        x >>= 1; // now 1 < x < 2
        res += ONE << (i - 1);
    }

    unchecked {
        --i;
    }
}

[2] if(_totalSupply > _initialTokenSupply) assures that _totalSupply - _initialTokenSupply will not underflow

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L378

uint256 _tokensPrimaryCurve;

unchecked {
    _tokensPrimaryCurve = _totalSupply - _initialTokenSupply;
}

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L319

unchecked {
    _purchaseReturn = _initialTokenSupply - _totalSupply;
}

[3] primaryReserveBalance - _saleReturn will not underflow

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L380

primaryReserveBalance -= _saleReturn is equivalent with primaryReserveBalance = primaryReserveBalance - _saleReturn which is same with primaryReserveBalance = primaryReserveBalance - (primaryReserveBalance - fictitiousPrimaryReserveBalance) = fictitiousPrimaryReserveBalance. Therefore primaryReserveBalance -= _saleReturn can be written like this:

unchecked {
    primaryReserveBalance -= _saleReturn;
}

[4] if (_buyoutBid > _currentValuation) assures that _buyoutBid - _currentValuation will not underflow

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L415

if (_buyoutBid > _currentValuation) {
    unchecked {
        safeTransferETH(payable(msg.sender), (_buyoutBid - _currentValuation));
    }
}

[5] require(... && _sellAmount <= _supply) assures that _supply - _sellAmount will not underflow

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L235

uint256 baseD;
unchecked {
    baseD = _supply - _sellAmount;
}

---

[GAS-4] Use custom errors

Using custom errors can reduce the gas cost.

There are more than 40 callsites which use require with inline string.

---

[GAS-5] Usage of mul and add function of SafeMath library at BancorFormula.sol increases the gas cost

In OpenZeppelin's SafeMath library, mul function simply multiply and add function do addition. mul function is only used in BancorFormula.sol, so not using mul or add function and removing the usage of SafeMath library can decrease the gas cost.

Here are OZ's definitions.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/math/SafeMath.sol#L93-L95

    function add(uint256 a, uint256 b) internal pure returns (uint256) {
        return a + b;
    }

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/math/SafeMath.sol#L121-L123

    function mul(uint256 a, uint256 b) internal pure returns (uint256) {
        return a * b;
    }

• Usage of add function

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L200

• Usages of mul function

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L196

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L202

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L231

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Bancor/BancorFormula.sol#L237