Nibbl contest - ElKu's results

1. NibblVaultFactory has timelock mechanism to update admin fees, basket implementation, vault implementation, adminfee address etc. I suggest that the proposal should have an expiry field too. For example:

vaultExpireTime = block.timestamp + UPDATE_TIME + EXPIRE_TIME;
require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime && block.timestamp <= vaultExpireTime, "Error");

This removes the possibility of an old proposal suddenly getting executed. Circumstances might have changed from the time when the proposal was made.

I also recommend to emit an event when a proposal is made. Another point to note is that, an older proposal is overwritten by a new one without any warning or checks at the moment. Not sure if the developers intended it to work this way.

References are: a. proposeNewBasketImplementation b. proposeNewAdminFeeAddress c. proposeNewAdminFee d. proposeNewVaultImplementation

1. function getTwavObservations() public view returns(TwavObservation[TWAV_BLOCK_NUMBERS] memory)

This function could be declared as external instead of public to save gas. As its not called from within the contract.

2. The require statements can be put on top if it's conditions arent going to be changed by the logic above it. Line number 350 and 351 can be swapped safely. It can save a storage write(saves 20000 gas) in some cases.

secondaryReserveBalance = _secondaryReserveBalance - _saleReturn;
require(_secondaryReserveBalance - _saleReturn >= MIN_SECONDARY_RESERVE_BALANCE, "NibblVault: Excess sell");

3. Cache the storage variables to memory to save gas. Storage read is 100 gas while memory read is only 3 gas. Reference: twavObservationsIndex in function _updateTWAV This saves around 200-6=194 gas.

4. There are three for loops in Basket.sol which could be gas optimized. a. i need not be initialized to 0 as its default value is 0. b. i++ could be put inside an unchecked block safely. c. _token.length could be cached to save gas.

For example

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

could be rewritten as:

 uint256 tokenLength = _tokens.length; 
 for (uint256 i; i < tokenLength; ) {
            //logic
	unchecked {
		i++;
	}
     }

The for loops are in line 43, 70 and 93.

5. Use custom error reporting instead of require statements. This reduces both deployment and runtime gas usage. See this link for an example. Also many of these errors are of the same type(especially in Basket.sol), which means we can use the same custom error for them, reducing the gas usage even further.

References where this could be done are:

a. In Basket.sol:  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");]

on lines 36 , 42 , 53 , 62 , 69 , 79 , 86 and 92.

b. NibblVault.sol: 

on lines 129 , 139 , 146-147 , 154 , 184-185 , 325 , 351 , 387 , 399-400 , 404 , 444 , 475 , 486 , 496 , 505 , 516 , 524 , 536 , 546 , 561 , 564 and 570.

c. NibblVaultFactory.sol: 

on lines 48-49 , 107 , 131 , 141 , 149 and 166.

d. AccessControlMechanism.sol:

on line 48.