Nibbl contest - StErMi's results

INibblVaultFactory.sol

Fractionalise event parameters are not declared as indexed

Some events are missing the indexed keyword on addresses. Using the indexed parameters allow external monitoring tool to filter those logs and monitor events in a better way.

• Fractionalise can declare assetAddress and proxyVault as indexed

Consider declaring those event parameters as indexed to better monitor/filter those events.

NibblVaultFactoryData.sol

Natspec documentation partially added or missing at all

All the state variables declared inside NibblVaultFactoryData contract are missing natspec documentation.

Natspec documentation are useful for internal developers that need to work on the project, external developers that need to integrate with the project, auditors that have to review it but also for end users given that Etherscan has officially integrated the support for it directly on their site.

Consider adding the missing natspec documentation.

Basket.sol

Natspec documentation partially added or missing at all

Some functions are missing at all the natspec documentation or have partially implemented it.

Natspec documentation are useful for internal developers that need to work on the project, external developers that have to integrate with the project, auditors that have to review it but also for end users given that Etherscan has officially integrated the support for it directly on their site.

The following functions are missing at all the natspec documentation or have partially implemented it:

• initialise miss all natspec
• withdrawERC721 missing natspec for @param _to
• withdrawMultipleERC721 missing all natspec
• withdrawERC721Unsafe missing natspec for @param _to
• withdrawERC1155 missing natspec for @param _to
• withdrawMultipleERC1155 miss all natspec
• withdrawETH missing natspec for @param _to
• withdrawERC20 missing natspec for @param _token
• withdrawMultipleERC20 missing all natspec
• onERC721Received missing all natspec
• onERC1155Received missing all natspec
• onERC1155BatchReceived missing all natspec

Consider adding the missing natspec documentation.

Basket contract has misspelled the initialize initialization function

The "standard" and official name for the initialization method is initialize (implemented by correctly by NibblVault).

The current name used by the Basket contract is initialise. Consider renaming the initialization function to initialize.

withdrawETH is not checking the amount of ETH to withdraw

The current implementation of withdrawETH is not checking the amount of ETH present in the Basket contract. Without any check, it allows the Basket owner / spender to withdraw 0 ETH and waste gas by doing so.

Consider implementing a check that performs the operation only when address(this).balance > 0

withdrawERC20, withdrawMultipleERC20, withdrawERC1155, withdrawMultipleERC1155 are not checking the amount of tokens to withdraw

Similar to the previous problem for withdrawETH, those functions are not checking if the contract has at some amount to transfer.

The transfer should happen only if the amount of the token is > 0 to not waste gas.

Consider implementing a check that performs the operation only when TOKEN_INTERFACE(_tokens[i]).balanceOf(address(this) > 0 where the TOKEN_INTERFACE is IERC20 or IERC1155 depending on the function called.

Twav.sol

Natspec documentation partially added or missing at all

Some functions are missing at all the natspec documentation or have partially implemented it.

Natspec documentation are useful for internal developers that need to work on the project, external developers that have to integrate with the project, auditors that have to review it but also for end users given that Etherscan has officially integrated the support for it directly on their site.

The following functions are missing at all the natspec documentation or have partially implemented it:

• getTwavObservations miss all natspec

Consider adding the missing natspec documentation.

twavObservations and all _getTwav comments could be improved

Currently, the code has this natspec/dev comment

• twavObservations → //TWAV of last 4 Blocks
• _getTwav

/// @notice returns the TWAV of the last 4 blocks
/// @return _twav TWAV of the last 4 blocks

Interpreting from the comment, it seems that prices in that array are from the last 4 consecutive blocks. In reality, those are prices from different blocks (the TWAV is updated only when _blockTimestamp != lastBlockTimeStamp and buyout is not active or when buyout process is initialized) but not consecutive because the functions that trigger the update could be called in blocks with a block number not consecutive.

Recommendation: update the comment, clarifying that those prices could not be from consecutive blocks.

NibblVault.sol

_chargeFee should use _feeAdmin and not _adminFeeAmt to decide if safeTransferETH should be called

_adminFeeAmt is the admin fee % and _feeAdmin is the amount of fee to be sent to the admin calculated from _adminFeeAmt.

The _feeAdmin could be 0 (rounding) even if the _adminFeeAmt is greater than 0.

Recommendation: use _feeAdmin instead of _adminFeeAmt in

if(_adminFeeAmt > 0) {
	safeTransferETH(_factory, _feeAdmin);
}

_chargeFeeSecondaryCurve should use _feeAdmin and not _adminFeeAmt to decide if safeTransferETH should be called

_adminFeeAmt is the admin fee % and _feeAdmin is the amount of fee to be sent to the admin calculated from _adminFeeAmt.

The _feeAdmin could be 0 (rounding) even if the _adminFeeAmt is greater than 0.

Recommendation: use _feeAdmin instead of _adminFeeAmt in

if(_adminFeeAmt > 0) {
	safeTransferETH(_factory, _feeAdmin);
}

Natspec documentation partially added or missing at all

Some functions are missing at all the natspec documentation or have partially implemented it.

Natspec documentation are useful for internal developers that need to work on the project, external developers that have to integrate with the project, auditors that have to review it but also for end users given that Etherscan has officially integrated the support for it directly on their site.

The following functions are missing at all the natspec documentation or have partially implemented it:

• getMaxSecondaryCurveBalance missing natspec for @return
• _buyPrimaryCurve missing natspec for @param _totalSupply
• _buySecondaryCurve missing natspec for @param _totalSupply
• buy missing natspec for @return
• _sellPrimaryCurve missing natspec for @param _totalSupply
• sell missing natspec for @return
• initiateBuyout missing natspec for @return
• redeem missing natspec for @return and @param _to
• redeemCuratorFee missing natspec for @return
• permit missing all natspec
• safeTransferETH missing all natspec
• onERC721Received missing all natspec
• onERC1155Received missing all natspec
• onERC1155BatchReceived missing all natspec

Consider adding the missing natspec documentation.

NibblVaultFactory.sol

Natspec documentation partially added or missing at all

Some functions are missing at all the natspec documentation or have partially implemented it.

Natspec documentation are useful for internal developers that need to work on the project, external developers that have to integrate with the project, auditors that have to review it but also for end users given that Etherscan has officially integrated the support for it directly on their site.

The following functions are missing at all the natspec documentation or have partially implemented it:

• createBasket missing all natspec
• getBasketAddress missing all natspec

Consider adding the missing natspec documentation.

constructor is not checking user input

The constructor is not checking the user input value for the following inputs:

• _vaultImplementation should be != address(0)
• _feeTo should be != address(0)
• _basketImplementation should be != address(0)

Consider adding checks on those inputs.

proposeNewBasketImplementation miss event emission and input value check

The proposeNewBasketImplementation function is missing a check on _newBasketImplementation that should be != address(0) and is not emitting an event when the value has changed.

updateBasketImplementation miss event emission

The updateBasketImplementation function should emit an event when the value has changed.

withdrawAdminFee miss event emission and check on ETH amount

The withdrawAdminFee function should emit an event when the value has changed. The low level call should be made only when address(this).balance to not waste gas.

proposeNewAdminFeeAddress miss event emission and input value check

The proposeNewAdminFeeAddress function is missing a check on _newFeeAddress that should be != address(0) and is not emitting an event when the value has changed.

updateNewAdminFeeAddress miss event emission

The updateNewAdminFeeAddress function should emit an event when the value has changed.

proposeNewAdminFee miss event emission

The proposeNewAdminFee function should emit an event when the value has changed.

updateNewAdminFee miss event emission

The updateNewAdminFee function should emit an event when the value has changed.

proposeNewVaultImplementation miss event emission and input value check

The proposeNewVaultImplementation function is missing a check on _newFeeAddress that should be != address(0) and is not emitting an event when the value has changed.

updateVaultImplementation miss event emission

The updateVaultImplementation function should emit an event when the value has changed.

BancorFormula.sol

The contract is using SafeMath but the solidity compiler is 0.8.10

I couldn't find the official link for the original contract, but seeing the usage of SafeMath I would assume that the BancorFormula contract was created before Solidity 0.8.10 (version used by the project).

With Solidity 0.8.x they implemented

Arithmetic operations revert on underflow and overflow. You can use unchecked { ... } to use the previous wrapping behaviour. Checks for overflow are very common, so we made them the default to increase readability of code, even if it comes at a slight increase of gas costs.

This means that all the operations inside the contract that do not use the SafeMath functions will revert in case of an underflow/overflow instead of under/overflowing like they would have done before Solidity 0.8.

Recommendation: be sure to check all the math operations of the contract.

Twav.sol

timestamp in TwavObservation is declared as uint32 but cannot be packed with other variables

Usually, it makes sense to try to follow a Tight Variable Packing approach to not waste gas, but in this case, it's useless and will only cost more gas.

The timestamp variable inside the struct will anyway take a whole slot because after it's declaration we have cumulativeValuation that is a uint256.

Declaring timestamp as a uint256 will allow NibblVault to waste less gas because there is no more need to perform all the math + cast operations done on the timestamp (example: uint32 _blockTimestamp = uint32(block.timestamp % 2**32);).

Consider change the type of timestamp to a uint256