Nibbl contest - 0xf15ers's results

1.Use a single modifier to check the same condition in mutliple functions

• Multiple instances of _isApprovedOrOwner() can be checked using a single modifier in basket.sol

modifier isApprovedOnwer() {
    require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed")
}

function withdrawERC721(....) isApprovedOnwer{ ..}
function withdrawMultipleERC721(....) isApprovedOnwer{ ..}
function withdrawERC721Unsafe(....) isApprovedOnwer{ ..}
function withdrawERC1155(....) isApprovedOnwer{ ..}
........
......

• same in NibblVault.sol for require(msg.sender == bidder,"NibblVault: Only winner")s

2. Add descriptive revert string in "require()"

Some require statements dont have revert strings, using short descriptive message is good practice

./contracts/NibblVaultFactory.sol:114:        require(_success);


./contracts/Bancor/BancorFormula.sol:259:        require(_baseN < MAX_NUM);
./contracts/Bancor/BancorFormula.sol:356:        require(false);

3. Unbounded loops with external calls

Some external function take arrays from users and iterate thorugh them. If a user sends very large loop the transaction may be too big to fit in a single block. Checking for a resonable array size in such case is a best pracitce.

• In withdrawMultipleERC1155() , withdrawMultipleERC20(), etc..

4. Address(0) checks while setting adderss in initialize() in NibblVault.sol

Check for zero address in the initialize function, so it doesn't gets set to 0 accidently.

5. Address(0) checks for withdraw methods for ERC20, ERC721 and ERC1155

The token withdraw methods for ERC20, ERC721 and ERC1155 take _to address to which these tokens are transferred, but this address may be accidently sent to zero address and the tokens may be lost forever.

• In withdrawUnsettledBids, redeem, redeemCuratorFee, updateCurator, withdrawERC721, withdrawMultipleERC721, withdrawERC20, withdrawMultipleERC20, withdrawERC1155, withdrawMultipleERC1155

This is very important for critical role changes such as updateCurator()

6. buy() function is not making any external calls, so reentrancy guard lock maynot be needed

• In NibblVault.sol#L300

7. Unused receive() should revert

• If the intention is for the Ether to be used, the user should call another function, otherwise it should revert.

8. floating pragma in AccessControlMechanism.sol

• Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
• pragma solidity ^0.8.0;

1. Use prefix increment/decrement instead of postfix increment/decrement to save gas

Using prefix increment saves small amount of gas compared to postifix incremeent since they return the updated value and dont have to store intermediate value. Changing postfix increment to prefix increment will save gas in loops where this is used in every iteration.

/// Change
for (uint256 i = 0; i < _tokens.length; i++) {

/// To
for (uint256 i = 0; i < _tokens.length; ++i) {

./contracts/Basket.sol:43:        for (uint256 i = 0; i < _tokens.length; i++) {
./contracts/Basket.sol:70:        for (uint256 i = 0; i < _tokens.length; i++) {
./contracts/Basket.sol:93:        for (uint256 i = 0; i < _tokens.length; i++) {
./contracts/NibblVault.sol:506:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
./contracts/NibblVault.sol:525:        for (uint256 i = 0; i < _assets.length; i++) {
./contracts/NibblVault.sol:547:        for (uint256 i = 0; i < _assets.length; i++) {
./contracts/NibblVault.sol:562:        bytes32 structHash = keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonces[owner]++, deadline));
./contracts/Test/UpgradedNibblVault.sol:452:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
./contracts/Test/UpgradedNibblVault.sol:469:        for (uint256 i = 0; i < _assets.length; i++) {
./contracts/Test/UpgradedNibblVault.sol:486:        for (uint256 i = 0; i < _assets.length; i++) {
./contracts/Test/UpgradedNibblVault.sol:493:        upgradedNewVariable++;
./contracts/Test/UpgradedNibblVault.sol:515:        bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, value, nonces[owner]++, deadline));   

2. The length of the loop is calculated in every iteration, caching it can save gas

The loop termination condition evaluates the length of the loop on every iteration. Caching it will save gas.

/// Change
for (uint256 i = 0; i < _tokens.length; i++) {

/// To
uint256 len = _tokens.length;
for (uint256 i = 0; i < len; i++) {

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

3. Make public functions external

Some public functions are never callled in the contract itself, such functions can be converted to external to save some gas

./contracts/NibblVaultFactory.sol:69:        uint256 _initialTokenPrice) public view returns(address _vault) {
./contracts/NibblVaultFactory.sol:76:    function getVaults() public view returns(ProxyVault[] memory ) {
./contracts/NibblVaultFactory.sol:80:    function createBasket(address _curator, string memory _mix) public override returns(address)  {
./contracts/NibblVaultFactory.sol:88:    function getBasketAddress(address _curator, string memory _mix) public override view returns(address _basket) {

4. Use immutable for expressions using keccak256()

• Constant caculation are expresssions and calculated each time
• In AccessControlMechanism.sol#L12-L14, NibblVault.sol,

bytes32 public constant FEE_ROLE = keccak256("FEE_ROLE"); 
bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
bytes32 public constant IMPLEMENTER_ROLE = keccak256("IMPLEMENTER_ROLE");

bytes32 private constant PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)")

5. Revert strings with length > 32 bytes will need more deployment gas

Use of revert strings longer than 32 bytes will require extra deploymenht gas, minimize them to save gas on deployment

./contracts/NibblVaultFactory.sol:107:        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./contracts/NibblVaultFactory.sol:131:        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./contracts/NibblVaultFactory.sol:141:        require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");
./contracts/NibblVaultFactory.sol:149:        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./contracts/NibblVaultFactory.sol:166:        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

6. Use solidity custom errors

solidity 0.8.4 introduces custom errors which are cheaper than using revert strings in terms of gas Use the custom error patterns to reduce gas cost.

for eg.

  // Before
  require(condition, "Revert strings");

  // After
  error CustomError();
  if (!condition) {
    revert CustomError();
  }

more details can be found here