code423n4 commented 2 years ago

QA (LOW & NON-CRITICAL)

[L-01] Use of floating pragma

Floating Pragma used in AccessControlMechanism.sol. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. Reference

[L-02] Different and outdated compiler versions

The scoped contracts have different solidity compiler ranges (0.8.10,^0.8.10) referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior. Current Solidity version available is 0.8.15

[L-03] Missing of zero address checks for the immutables/constants

No address(0) or Zero value check at the constructors of: ProxyBasket.sol, ProxyVault.sol, AccessControlMechanism.sol, NibblVaultFactory.sol.

[L-04] External Calls inside a loop

Calls to external contracts inside a loop are dangerous (especially if the loop index can be user-controlled) because it could lead to DoS if one of the calls reverts or execution runs out of gas. Avoid calls within loops, check that loop index cannot be user-controlled or is bounded.Reference

Instances in Basket.sol;

Instances in NibblVault.sol;

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L526

[L-05] ecrecover() function

The contract uses ecrecover() function in NibblVault contract. EVM's ecrecover is susceptible to signature malleability which allows replay attacks, but using OpenZeppelin’s ECDSA library can be mitigation in NibbleVault.sol for permit() function. Reference

[L-06] Missing logic

In NibblVault.sol, to reach the deadline the logic should be;

139:        require(buyoutEndTime >= block.timestamp || buyoutEndTime == 0,'NibblVault: Bought Out');

instead of;

139:         require(buyoutEndTime > block.timestamp || buyoutEndTime == 0,'NibblVault: Bought Out');

[L-07] Critical address / parameter changes

Critical address / value changes are not done in two steps for the following methods: In NibblVault.sol

updateCurator()

[L-08] NibblVault and Basket contract are not implementing EIP1155 standard

EIP1155 states that the onERC1155Received and onERC1155BatchReceived MUST NOT be called on an EOA (Externally Owned Account). Reference The contract can implement isContract() function to fulfill this.

[L-09] Missing 0 address check at ecrecover()

The require statement is missing at ecrecover()function in NibblVault.sol's permit() function. It can be amended as below;

require(owner!=address(0),"err");
address signer = ecrecover(toTypedMessageHash(structHash), v, r, s);
require(signer == owner, "NibblVault: invalid signature");

[N-01] Scoped contracts are missing proper NatSpec comments

The scoped contracts Basket.sol, NibblVault.sol are missing proper NatSpec comments such as @notice @dev @param on many places.It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI) Reference