Nibbl contest - TomJ's results

[N-01] Event is Missing Indexed Fields

Each event should have 3 indexed fields if there are 3 or more fields.

Issue found at

./Basket.sol:15:    event DepositERC721(address indexed token, uint256 tokenId, address indexed from);
./Basket.sol:16:    event WithdrawERC721(address indexed token, uint256 tokenId, address indexed to);
./Basket.sol:17:    event DepositERC1155(address indexed token, uint256 tokenId, uint256 amount, address indexed from);
./Basket.sol:18:    event DepositERC1155Bulk(address indexed token, uint256[] tokenId, uint256[] amount, address indexed from);
./Basket.sol:19:    event WithdrawERC1155(address indexed token, uint256 tokenId, uint256 amount, address indexed from);

[N-02] Best Practice for Short Function Layout

Some contract is not following best practice of short function layout. This will reduce readability of code and harder to audit.

Following is line from solidity docs (link: https://docs.soliditylang.org/en/v0.8.15/style-guide.html#function-declaration)

The opening brace should be preceded by a single space.

Some of the function doesn't have this single space. Issue found at NibblVault.sol

NibblVault.sol#L252 NibblVault.sol#L347 NibblVault.sol#L464 NibblVaultFactory.sol#L80 Twav.sol#L35

[N-03] Best Practice of Long Function Layout

Some contract is not following best practice of long function layout. This will reduce readability of code and harder to audit.

Following is line from solidity docs (link: https://docs.soliditylang.org/en/v0.8.15/style-guide.html#function-declaration)

For long function declarations, it is recommended to drop each argument onto its own line 
at the same indentation level as the function body. The closing parenthesis and opening bracket 
should be placed on their own line as well at the same indentation level as the function declaration.

Some of the function doesn't have closing parenthesis and opening bracket on their own line. Issue found at NibblVault.sol

NibblVault.sol#L182 NibblVault.sol#L560 NibblVaultFactory.sol#L47 NibblVaultFactory.sol#L69

[N-04] TYPO

Some typo was found in the following

1. NibblVault.sol

250: /// @dev The max continous tokens on SecondaryCurve is equal to initialTokenSupply should be continuous

263: /// @dev Valuation = If current supply is on seconday curve we use secondaryReserveBalance and should be secondary

270: /// @param _amount amount of reserve tokens to buy continous tokens should be continuous

282: /// @param _amount amount of reserve tokens to buy continous tokens should be continuous

361: /// @param _to Address to recieve the reserve token to should be receive

2. ProxyBaseket.sol

26: * This function does not return to its internall call site, it will return directly to the external caller. should be internal

3. ProxyVault.sol

26: * This function does not return to its internall call site, it will return directly to the external caller. should be internal

[N-05] Require Statements without Descriptive Revert Strings

It is best practice to include descriptive revert strings for require statement for readability and auditing.

Issue found at

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

[N-06] Wrong NatSpec

There is mistake in following NatSpec. I think it was copied from withdrawERC721 function.

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L58 This NatSpec says "withdraw an ERC721 token" but it should be ERC1155.

[N-07] Incomplete NatSpec

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces. However there were places where this NatSpec was incomplete.

Issue found at

1. no @param for _to https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L77

2. no @param for _token https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L84

3. no NatSpec at all https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L99-L112

4. no @return https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L252

[G-01] Constant Value of a Call to keccak256() should Use Immutable

When using constant it is expected that the value should be converted into a constant value at compile time. However when using a call to keccak256(), the expression is re-calculated each time the constant is referenced. Resulting in costing about 100 gas more on each access to this "constant". link for more details: https://github.com/ethereum/solidity/issues/9232

Issue found at

./EIP712Base.sol:7-10:    bytes32 internal constant EIP712_DOMAIN_TYPEHASH = keccak256(bytes("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"));
./NibblVault.sol:51:    bytes32 private constant PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");
./AccessControlMechanism.sol:12:    bytes32 public constant FEE_ROLE = keccak256("FEE_ROLE");
./AccessControlMechanism.sol:13:    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
./AccessControlMechanism.sol:14:    bytes32 public constant IMPLEMENTER_ROLE = keccak256("IMPLEMENTER_ROLE");

[G-02] Unnecessary Default Value Initialization

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. link: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

I suggest removing default value initialization for following variables.

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

For example these can change to:

• for (uint256 i; i < _tokens.length; i++) {

[G-03] Store Array's Length as a Variable

I suggest to store an array's length as a variable before the for-loop since it can save 3 gas per iteration.

Issue found at:

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

For example, I suggest changing it to

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

[G-04] ++i Costs Less Gas than i++

It is better to use ++i than i++ when possible since it costs less gas.

Issue found at:

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

[G-05] Use Calldata instead of Memory for Read Only Function Parameters

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

I recommend changing following memory to calldata

./EIP712Base.sol:15:    function INIT_EIP712(string memory name, string memory version) internal {
./Basket.sol:41:    function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external override {
./Basket.sol:68:    function withdrawMultipleERC1155(address[] memory _tokens, uint256[] memory _tokenIds, address _to) external override {
./Basket.sol:91:    function withdrawMultipleERC20(address[] memory _tokens) external override {
./Basket.sol:99:    function onERC721Received(address, address from, uint256 id, bytes memory) external override returns(bytes4) {
./Basket.sol:104:    function onERC1155Received(address, address from, uint256 id, uint256 amount, bytes memory) external virtual override returns (bytes4) {
./Basket.sol:109:    function onERC1155BatchReceived(address, address from, uint256[] memory ids, uint256[] memory amounts, bytes memory) external virtual override returns (bytes4) {
./NibblVault.sol:174:        string memory _tokenName, 
./NibblVault.sol:175:        string memory _tokenSymbol, 
./NibblVault.sol:504:    function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {
./NibblVault.sol:523:    function withdrawMultipleERC20(address[] memory _assets, address _to) external override boughtOut {
./NibblVault.sol:545:    function withdrawMultipleERC1155(address[] memory _assets, uint256[] memory _assetIDs, address _to) external override boughtOut {
./NibblVault.sol:577:    function onERC1155Received(address, address, uint256, uint256, bytes memory) external pure returns (bytes4) {
./NibblVault.sol:581:    function onERC1155BatchReceived(address, address, uint256[] memory, uint256[] memory, bytes memory) external pure returns (bytes4) {
./NibblVaultFactory.sol:41:        string memory _name,
./NibblVaultFactory.sol:42:        string memory _symbol,
./NibblVaultFactory.sol:80:    function createBasket(address _curator, string memory _mix) public override returns(address)  {
./NibblVaultFactory.sol:88:    function getBasketAddress(address _curator, string memory _mix) public override view returns(address _basket) {

[G-06] Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

This is because EVM operates on 32 bytes at a time. So I recommend using uint256 instead of anything smaller. More information about this in the following link. link: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

Issue found at

./Twav.sol:6:        uint32 timestamp;
./Twav.sol:11:    uint8 public twavObservationsIndex;
./Twav.sol:12:    uint8 private constant TWAV_BLOCK_NUMBERS = 4; //TWAV of last 4 Blocks 
./Twav.sol:13:    uint32 public lastBlockTimeStamp;
./Twav.sol:21:    function _updateTWAV(uint256 _valuation, uint32 _blockTimestamp) internal {
./Twav.sol:22:        uint32 _timeElapsed; 
./Twav.sol:37:            uint8 _index = ((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS;
./NibblVault.sol:28:    uint32 private constant primaryReserveRatio = 200_000; //20%
./NibblVault.sol:57:    uint32 public secondaryReserveRatio;
./NibblVault.sol:183:        uint32 _secondaryReserveRatio = uint32((msg.value * SCALE * 1e18) / (_initialTokenSupply * _initialTokenPrice));
./NibblVault.sol:303:            uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
./NibblVault.sol:365:            uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
./NibblVault.sol:445:        uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
./NibblVault.sol:557:        uint8 v,

[G-07] Use require instead of &&

When there are multiple conditions in require statement, break down the require statement into multiple require statements instead of using && can save gas.

Issue found at

./NibblVaultFactory.sol:107:        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./NibblVaultFactory.sol:131:        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./NibblVaultFactory.sol:149:        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./NibblVaultFactory.sol:166:        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

For example these can be changed to

require(basketUpdateTime != 0);
require(block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

[G-08] Reduce the Long Revert Strings of Error Messages

By keeping the revert strings within 32 bytes will save you gas since each slot is 32 bytes.

Following are revert strings that are more than 32 bytes.

./NibblVaultFactory.sol:48:        require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");
./NibblVaultFactory.sol:49:        require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");
./NibblVaultFactory.sol:107:        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./NibblVaultFactory.sol:131:        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./NibblVaultFactory.sol:141:        require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");
./NibblVaultFactory.sol:149:        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./NibblVaultFactory.sol:166:        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

[G-09] Duplicate require() Checks Should be a Modifier or a Function

Since below require checks are used more than once, I recommend making these to a modifier or a function.

./Basket.sol:36:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:42:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:53:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:62:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:69:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:79:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:86:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:92:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");

./NibblVault.sol:475:        require(msg.sender == curator,"NibblVault: Only Curator");
./NibblVault.sol:486:        require(msg.sender == curator,"NibblVault: Only Curator");

./NibblVault.sol:496:        require(msg.sender == bidder,"NibblVault: Only winner");
./NibblVault.sol:505:        require(msg.sender == bidder,"NibblVault: Only winner");
./NibblVault.sol:516:        require(msg.sender == bidder, "NibblVault: Only winner");
./NibblVault.sol:524:        require(msg.sender == bidder, "NibblVault: Only winner");
./NibblVault.sol:536:        require(msg.sender == bidder, "NibblVault: Only winner");
./NibblVault.sol:546:        require(msg.sender == bidder, "NibblVault: Only winner");

[G-10] Use Custom Errors to Save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

I recommend using custom errors.

./Basket.sol:36:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:42:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:53:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:62:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:69:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:79:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:86:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./Basket.sol:92:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
./NibblVault.sol:129:        require(unlocked == 1, 'NibblVault: LOCKED');
./NibblVault.sol:139:        require(buyoutEndTime > block.timestamp || buyoutEndTime == 0,'NibblVault: Bought Out');
./NibblVault.sol:146:        require(status == Status.buyout, "NibblVault: status != buyout");
./NibblVault.sol:147:        require(buyoutEndTime <= block.timestamp, "NibblVault: buyoutEndTime <= now");
./NibblVault.sol:154:        require(!NibblVaultFactory(factory).paused(), 'NibblVault: Paused');
./NibblVault.sol:184:        require(_secondaryReserveRatio <= primaryReserveRatio, "NibblVault: Excess initial funds");
./NibblVault.sol:185:        require(_secondaryReserveRatio >= MIN_SECONDARY_RESERVE_RATIO, "NibblVault: secResRatio too low");
./NibblVault.sol:325:        require(_minAmtOut <= _purchaseReturn, "NibblVault: Return too low");
./NibblVault.sol:351:        require(_secondaryReserveBalance - _saleReturn >= MIN_SECONDARY_RESERVE_BALANCE, "NibblVault: Excess sell");
./NibblVault.sol:387:        require(_saleReturn >= _minAmtOut, "NibblVault: Return too low");
./NibblVault.sol:399:        require(block.timestamp >= minBuyoutTime, "NibblVault: minBuyoutTime < now");
./NibblVault.sol:400:        require(status == Status.initialized, "NibblVault: Status!=initialized");
./NibblVault.sol:404:        require(_buyoutBid >= _currentValuation, "NibblVault: Bid too low");
./NibblVault.sol:444:        require(status == Status.buyout, "NibblVault: Status!=Buyout");
./NibblVault.sol:475:        require(msg.sender == curator,"NibblVault: Only Curator");
./NibblVault.sol:486:        require(msg.sender == curator,"NibblVault: Only Curator");
./NibblVault.sol:496:        require(msg.sender == bidder,"NibblVault: Only winner");
./NibblVault.sol:505:        require(msg.sender == bidder,"NibblVault: Only winner");
./NibblVault.sol:516:        require(msg.sender == bidder, "NibblVault: Only winner");
./NibblVault.sol:524:        require(msg.sender == bidder, "NibblVault: Only winner");
./NibblVault.sol:536:        require(msg.sender == bidder, "NibblVault: Only winner");
./NibblVault.sol:546:        require(msg.sender == bidder, "NibblVault: Only winner");
./NibblVault.sol:561:        require(block.timestamp <= deadline, "NibblVault: expired deadline");
./NibblVault.sol:564:        require(signer == owner, "NibblVault: invalid signature");
./NibblVault.sol:570:        require(success, "NibblVault: ETH transfer failed");
./NibblVaultFactory.sol:48:        require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");
./NibblVaultFactory.sol:49:        require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");
./NibblVaultFactory.sol:107:        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./NibblVaultFactory.sol:131:        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./NibblVaultFactory.sol:141:        require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");
./NibblVaultFactory.sol:149:        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./NibblVaultFactory.sol:166:        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
./AccessControlMechanism.sol:48:        require(pendingRoles[_role][msg.sender], "AccessControl: Role not pending");

[G-11] Defined Variables Used Only Once

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

Issue found at

1. NibblVaultFactory.sol

• Remove "newsalt", "code" and "_hash" variable of getVaultAddress function

70:        bytes32 newsalt = keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID,  _initialSupply, _initialTokenPrice));
71:        bytes memory code = abi.encodePacked(type(ProxyVault).creationCode, uint256(uint160(address(this))));
72:        bytes32 _hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)));
73:        _vault = address(uint160(uint256(_hash)));     

• Remove "newsalt", "code" and "hash" variable of getBasketAddress function

89:        bytes32 newsalt = keccak256(abi.encodePacked(_curator, _mix));
90:        bytes memory code = abi.encodePacked(type(ProxyBasket).creationCode, uint256(uint160(basketImplementation)));
91:        bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)));
92:        _basket = address(uint160(uint256(hash)));     

2. Twav.sol

• Remove "_prevCumulativeValuation" variable of _updateTWAV function

27:        uint256 _prevCumulativeValuation = twavObservations[((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS].cumulativeValuation;
28:        twavObservations[twavObservationsIndex] = TwavObservation(_blockTimestamp, _prevCumulativeValuation + (_valuation * _timeElapsed)); //add the previous observation to make it cumulative