Nibbl contest - sashik_eth's results

L01 - Lack of event emitting after sensitive actions

Contracts do not emit relevant events after setting sensitive variables. Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity in following functions:

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

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L106-L110

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L130-L134

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L148-L152

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L165-169

N01 - No needed import of SafeMath

contracts/NibblVaultFactory.sol:9   import { SafeMath } from  "@openzeppelin/contracts/utils/math/SafeMath.sol";

N02 - Typos

contracts/NibblVault.sol:125 ///@notice reenterancy guard contracts/NibblVault.sol:152 /// @dev pausablity implemented in factory contracts/NibblVault.sol:200 //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator contracts/NibblVault.sol:263 /// @dev Valuation = If current supply is on seconday curve we use secondaryReserveBalance and secondaryReserveRatio to calculate valuation else we use primary reserve ratio and balance contracts/NibblVault.sol:512 /// @notice ERC20s can be accumulated by the underlying ERC721 in the vault as royalty or airdops contracts/Proxy/ProxyBasket.sol:26 * This function does not return to its internall call site, it will return directly to the external caller. contracts/Proxy/ProxyVault.sol:26 * This function does not return to its internall call site, it will return directly to the external caller.

N03 - Constant naming

Constants should be named with all capital letters with underscores separating words.

contracts/NibblVault.sol:28 uint32 private constant primaryReserveRatio = 200_000; //20%  

G01 - More gas efficient for loop with ++i increment and unchecked block

Loop index increments can be written as unchecked { ++i } instead of simply i++ to save gas.

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/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++) { 

G02 - unchecked block can be used for gas efficiency of the expression that can't overflow/underflow

L415 could be unchecked due to check on L414

contracts/NibblVault.sol:414    if (_buyoutBid > _currentValuation) {
contracts/NibblVault.sol:415        safeTransferETH(payable(msg.sender), (_buyoutBid - _currentValuation)); // @audit gas unchecked L404
contracts/NibblVault.sol:416    }

L319 could be unchecked due to check on L311

L322 could be unchecked due to check on L315 And _totalSupply + _purchaseReturn on L315 could be substituted with _initialTokenSupply since _purchaseReturn = _initialTokenSupply - _totalSupply; (L319)

L351 could be unchecked since it's would not underflow, on L350 would check it.

L378 could be unchecked due to check on L373

L428 could be unchecked since would be check for overflow on L429 and any user unsettledBids could not be bigger than totalUnsettledBids

Lines with adding block.timestamp value to constant value could be unchecked since their overflow would appear only in the extremely far future:

contracts/NibblVault.sol:411    buyoutEndTime = block.timestamp + BUYOUT_DURATION; 
contracts/NibblVaultFactory.sol:101 basketUpdateTime = block.timestamp + UPDATE_TIME; 
contracts/NibblVaultFactory.sol:125 feeToUpdateTime = block.timestamp + UPDATE_TIME; 
contracts/NibblVaultFactory.sol:143 feeAdminUpdateTime = block.timestamp + UPDATE_TIME;  
contracts/NibblVaultFactory.sol:160 vaultUpdateTime = block.timestamp + UPDATE_TIME;

L457 could be unchecked since totalUnsettledBids always >= than any user unsettledBids .

    function withdrawUnsettledBids(address payable _to) external override {
        uint _amount = unsettledBids[msg.sender];
        delete unsettledBids[msg.sender];
        totalUnsettledBids -= _amount;
        safeTransferETH(_to, _amount);
    }

G03 - Avoid long revert strings

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. Next revert strings are longer than 32 bytes:

contracts/NibblVaultFactory.sol:48  require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low"); 
contracts/NibblVaultFactory.sol:49  require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender"); 
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");  

G04 - Admin role setup

No need to setup role admin with DEFAULT_ADMIN_ROLE since due to openzeppelin docs: By default, the admin role for all roles is DEFAULT_ADMIN_ROLE

    constructor (address _admin) {
        bytes32 _defaultAdminRole = DEFAULT_ADMIN_ROLE;
        _grantRole(_defaultAdminRole, _admin);
        _setRoleAdmin(_defaultAdminRole, _defaultAdminRole); // @audit gas needed? here and below
        _setRoleAdmin(FEE_ROLE, _defaultAdminRole); 
        _setRoleAdmin(PAUSER_ROLE, _defaultAdminRole);
        _setRoleAdmin(IMPLEMENTER_ROLE, _defaultAdminRole);
    }

G05 - Use previous index in _getTwav()

Since in _getTwav() on L39 index for previous observation will always be the same as current twavObservationsIndex there is no need to count it. twavObservationsIndex value could be cached and user on L37 and L39, like next:

    function _getTwav() internal view returns(uint256 _twav){
        if (twavObservations[TWAV_BLOCK_NUMBERS - 1].timestamp != 0) {
-           uint8 _index = ((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS;
+           uint8 _prevIndex = twavObservationsIndex; 
+           uint8 _index = ((_prevIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS;
            TwavObservation memory _twavObservationCurrent = twavObservations[(_index)];
-           TwavObservation memory _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];
+	    TwavObservation memory _twavObservationPrev = twavObservations[_prevIndex];
            _twav = (_twavObservationCurrent.cumulativeValuation - _twavObservationPrev.cumulativeValuation) / (_twavObservationCurrent.timestamp - _twavObservationPrev.timestamp);
        }
    }

G06 - No needed operations

Next lines:

contracts/NibblVault.sol:379    _saleReturn = primaryReserveBalance - fictitiousPrimaryReserveBalance;
contracts/NibblVault.sol:380    primaryReserveBalance -= _saleReturn;

Equivalent to:

primaryReserveBalance = fictitiousPrimaryReserveBalance;