Nibbl contest - MiloTruck's results

QA Report

Low Issues

Front-runnable initializers

In the NibblVault and Basket contracts, the initialize functions are missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, an attacker could supply incorrect parameters, leaving the contract needing to be redeployed.

Proof of Concept The initialize functions in the following contracts have missing access controls:

1. https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L173-L204
2. https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L23-L25

Recommended Mitigation Steps Setting the owner in the contract's constructor to msg.sender and adding an onlyOwner modifier to all initializers would be a sufficient level of access control to prevent front-running.

Non-Critical Issues

lock modifier should be used before any other modifier

The function buy() uses the modifier notBoughtOut before lock, as shown below:

contracts/NibblVault.sol:
 300:        function buy(uint256 _minAmtOut, address _to) external override payable notBoughtOut lock whenNotPaused returns(uint256 _purchaseReturn) {

Similar to OpenZeppelin's nonReentrant modifier, the lock modifier should always be the first modiifer for all functions. In this case, it lock does not protect against reentrancy during the execution of the first modifier. Although the current implementation of notBoughtOut is not vulnerable to reentrancy, it is still a good practice to use lock as the first modifier.

Consider using lock as the first modifier:

contracts/NibblVault.sol:
 300:        function buy(uint256 _minAmtOut, address _to) external override payable lock notBoughtOut whenNotPaused returns(uint256 _purchaseReturn) {

Missing error message in require statements

It is recommended to have error messages for all require statements to provide users with information should a transaction fail.

Consider adding an error message to the following require statements:

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

BUYOUT_DURATION in NibblVault.sol differs from documentation

In the documenation provided, it states that the buyout rejection period is 3 days long, as shown below:

The buyout would get rejected if in any n day period (n=3 in our current implementation) starting from the time the buyout bid is placed, the time weighted average valuation (TWAV) becomes greater than or equal to buyoutRejectionValuation.

The buyout would be successful if, in a certain predefined time (3 days in our current implementation) starting from the time the buyout bid is placed, the time-weighted average valuation (TWAV) doesn’t go above the buyoutRejectionValuation.

However, in NibblVault.sol, BUYOUT_DURATION is set to 5 days:

contracts/NibblVault.sol:
  36:        /// @notice The days until which a buyout bid is valid, if the bid isn't rejected in buyout duration time, its automatically considered boughtOut
  37:        uint256 private constant BUYOUT_DURATION = 5 days; 

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommended Mitigation Steps
Block timestamps should not be used for entropy or generating random numbers — i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Instances where block.timestamp is used:

contracts/NibblVault.sol:
 139:        require(buyoutEndTime > block.timestamp || buyoutEndTime == 0,'NibblVault: Bought Out');
 147:        require(buyoutEndTime <= block.timestamp, "NibblVault: buyoutEndTime <= now");
 303:        uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
 365:        uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
 399:        require(block.timestamp >= minBuyoutTime, "NibblVault: minBuyoutTime < now");
 411:        buyoutEndTime = block.timestamp + BUYOUT_DURATION;
 413:        _updateTWAV(_currentValuation, uint32(block.timestamp % 2**32));
 445:        uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
 561:        require(block.timestamp <= deadline, "NibblVault: expired deadline");

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

Floating compiler versions

Non-library/interface files should use fixed compiler versions, not floating ones:

contracts/Utilities/AccessControlMechanism.sol:
   4:        pragma solidity ^0.8.0;

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields:

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

Gas Optimizations Report

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

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

can be changed to:

for (uint256 i; i < length; ++i) {

Consider declaring the following lines without explicitly setting the index to 0:

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

contracts/Basket.sol:
  43:        for (uint256 i = 0; i < _tokens.length; i++) {
  70:        for (uint256 i = 0; i < _tokens.length; i++) {
  93:        for (uint256 i = 0; i < _tokens.length; i++) {

For-Loops: Cache array length outside of loops

Reading an array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

For example:

for (uint256 i; i < arr.length; ++i) {}

can be changed to:

uint256 len = arr.length;
for (uint256 i; i < len; ++i) {}

Consider making the following change to these lines:

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

contracts/Basket.sol:
  43:        for (uint256 i = 0; i < _tokens.length; i++) {
  70:        for (uint256 i = 0; i < _tokens.length; i++) {
  93:        for (uint256 i = 0; i < _tokens.length; i++) {

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

For example, the code below:

for (uint256 i; i < numIterations; ++i) {  
    // ...  
}  

can be changed to:

for (uint256 i; i < numIterations;) {  
    // ...  
    unchecked { ++i; }  
}  

Consider making the following change to these lines:

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

contracts/Basket.sol:
  43:        for (uint256 i = 0; i < _tokens.length; i++) {
  70:        for (uint256 i = 0; i < _tokens.length; i++) {
  93:        for (uint256 i = 0; i < _tokens.length; i++) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

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

contracts/Basket.sol:
  43:        for (uint256 i = 0; i < _tokens.length; i++) {
  70:        for (uint256 i = 0; i < _tokens.length; i++) {
  93:        for (uint256 i = 0; i < _tokens.length; i++) {

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

contracts/NibblVault.sol:
 227:        if(_adminFeeAmt > 0) {
 243:        if(_adminFeeAmt > 0) {

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

contracts/NibblVaultFactory.sol:
  64:        function getVaultAddress(
  65:            address _curator,
  66:            address _assetAddress,
  67:            uint256 _assetTokenID,
  68:            uint256 _initialSupply,
  69:            uint256 _initialTokenPrice) public view returns(address _vault) {

  76:        function getVaults() public view returns(ProxyVault[] memory ) {

contracts/Twav/Twav.sol:
  44:        function getTwavObservations() public view returns(TwavObservation[TWAV_BLOCK_NUMBERS] memory) {

Errors: Reduce the length of error messages (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 is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

In these instances, consider shortening the revert strings to fit within 32 bytes, or using custom errors:

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

Errors: Use modifiers instead of require statements duplicate access role checks

Instead of using a require statement to check that msg.sender belongs to a certain role (e.g. msg.sender is owner), consider using modifiers. This would help to save gas and improve code clarity.

For example, to check that msg.sender is owner, a modifier can be written as such:

modifier isOwner() {
  require(msg.sender == owner, "error");
  _;
}

Functions can then use isOwner to validate msg.sender, for example:

function setOwner(address _owner) external {
  require(msg.sender == owner, "error");
  // ...
}

can be rewritten to:

function setOwner(address _owner) external isOwner {
  // ...
}

Instances where modifiers can be used include:

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

Errors: Use multiple require statements instead of &&

Instead of using a single require statement with the && operator, using multiple require statements would help to save runtime gas cost. However, note that this results in a higher deployment gas cost, which is a fair trade-off.

A require statement can be split as such:

// Original code:
require(a && b, 'error');

// Changed to:
require(a, 'error: a');
require(b, 'error: b');

Instances where multiple require statements should be used:

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

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to:

• Cheaper deployment cost
• Lower runtime cost upon revert

Taken from Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors can be defined using of the error statement, both inside or outside of contracts.

Instances where custom errors can be used instead:

contracts/NibblVault.sol:
 129:        require(unlocked == 1, 'NibblVault: LOCKED');
 139:        require(buyoutEndTime > block.timestamp || buyoutEndTime == 0,'NibblVault: Bought Out');
 146:        require(status == Status.buyout, "NibblVault: status != buyout");
 147:        require(buyoutEndTime <= block.timestamp, "NibblVault: buyoutEndTime <= now");
 154:        require(!NibblVaultFactory(factory).paused(), 'NibblVault: Paused');
 184:        require(_secondaryReserveRatio <= primaryReserveRatio, "NibblVault: Excess initial funds");
 185:        require(_secondaryReserveRatio >= MIN_SECONDARY_RESERVE_RATIO, "NibblVault: secResRatio too low");
 325:        require(_minAmtOut <= _purchaseReturn, "NibblVault: Return too low");
 351:        require(_secondaryReserveBalance - _saleReturn >= MIN_SECONDARY_RESERVE_BALANCE, "NibblVault: Excess sell");
 387:        require(_saleReturn >= _minAmtOut, "NibblVault: Return too low");
 399:        require(block.timestamp >= minBuyoutTime, "NibblVault: minBuyoutTime < now");
 400:        require(status == Status.initialized, "NibblVault: Status!=initialized");
 404:        require(_buyoutBid >= _currentValuation, "NibblVault: Bid too low");
 444:        require(status == Status.buyout, "NibblVault: Status!=Buyout");
 475:        require(msg.sender == curator,"NibblVault: Only Curator");
 486:        require(msg.sender == curator,"NibblVault: Only Curator");
 496:        require(msg.sender == bidder,"NibblVault: Only winner");
 505:        require(msg.sender == bidder,"NibblVault: Only winner");
 516:        require(msg.sender == bidder, "NibblVault: Only winner");
 524:        require(msg.sender == bidder, "NibblVault: Only winner");
 536:        require(msg.sender == bidder, "NibblVault: Only winner");
 546:        require(msg.sender == bidder, "NibblVault: Only winner");
 561:        require(block.timestamp <= deadline, "NibblVault: expired deadline");
 564:        require(signer == owner, "NibblVault: invalid signature");
 570:        require(success, "NibblVault: ETH transfer failed");

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

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

contracts/Utilities/AccessControlMechanism.sol:
  48:        require(pendingRoles[_role][msg.sender], "AccessControl: Role not pending");

lock modifier in NibblVault.sol is unnecessary

The function buy() in NibblVault.sol uses the lock modifier to prevent reentrancy. However, this is unneeded as:

1. All external calls are made to trusted contracts and cannot be hooked for reentrancy
2. safeTransferETH is used to transfer ETH only to factory, which is assumed to be a trusted contract

As such, buy() is not vulnerable to reentrancy and does not need the lock modifier. Furthermore, as buy() is the only function that uses the lock modifier, it can be removed from the contract entirely. This would help to save runtime and deployment gas costs.

Unnecessary definition of variables

Some variables are defined even though they are only used once in their respective functions. Not defining these variables can help to reduce gas cost and contract size.

Instances include:

contracts/NibblVault.sol:
 378:        uint256 _tokensPrimaryCurve = _totalSupply - _initialTokenSupply;

Variables declared as constant are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232:

Consequences: each usage of a “constant” costs ~100 gas more on each access (it is still a little better than storing the result in storage, but not much). since these are not real constants, they can’t be referenced from a real constant environment (e.g. from assembly, or from another library)

contracts/NibblVault.sol:
  51:        bytes32 private constant PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");

contracts/Utilities/AccessControlMechanism.sol:
  12:        bytes32 public constant FEE_ROLE = keccak256("FEE_ROLE");
  13:        bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
  14:        bytes32 public constant IMPLEMENTER_ROLE = keccak256("IMPLEMENTER_ROLE");

contracts/Utilities/EIP712Base.sol:
   7:        bytes32 internal constant EIP712_DOMAIN_TYPEHASH = keccak256(
   8:            bytes(
   9:                "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
  10:            )
  11:        );

Change these expressions from constant to immutable and implement the calculation in the constructor. Alternatively, hardcode these values in the constants and add a comment to say how the value was calculated.

Use calldata instead of memory for read-only function parameters

If a reference type function parameter, such as arrays, is read-only, it is cheaper to use calldata instead of memory. This would help to save gas as values are read directly from calldata using calldataload and avoids additional intermediate memory operations.

Consider changing memory to calldata in the following functions:

contracts/Basket.sol:
  41:        function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external override {
  68:        function withdrawMultipleERC1155(address[] memory _tokens, uint256[] memory _tokenIds, address _to) external override {
  91:        function withdrawMultipleERC20(address[] memory _tokens) external override {
    
contracts/NibblVault.sol:
 504:        function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {
 523:        function withdrawMultipleERC20(address[] memory _assets, address _to) external override boughtOut {
 545:        function withdrawMultipleERC1155(address[] memory _assets, uint256[] memory _assetIDs, address _to) external override boughtOut {