As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”
Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”
Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:
40: function proposeGrantRole(bytes32 _role, address _to) external override onlyRole(getRoleAdmin(_role)) {
41: pendingRoles[_role][_to] = true;
42: }
43:
44: /// @notice proposed user needs to claim the role
45: /// @dev can only be called by the proposed user
46: /// @param _role role to be claimed
47: function claimRole(bytes32 _role) external override {
48: require(pendingRoles[_role][msg.sender], "AccessControl: Role not pending");
49: _grantRole(_role, msg.sender);
50: delete pendingRoles[_role][msg.sender];
51: }
7. TWAV is only over four blocks
Someone does not have to maintain price variation for long to reject a buyout. This can result in blocking of buyouts.
File: Twav.sol
12: uint8 private constant TWAV_BLOCK_NUMBERS = 4; //TWAV of last 4 Blocks
8. Unsafe casting may overflow
SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting.
Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:
Basic ecrecover is used, leading to being able to approve transfers from the zero address. Any tokens sent to 0 address can be recovered by another user.
11. Unsafe use of transfer()/transferFrom() with IERC20
Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead
12. Return values of transfer()/transferFrom() not checked
Not all IERC20 implementations revert() when there's a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment.
16. _safeMint() should be used rather than _mint() wherever possible
_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.
Basket.sol:6:import { ERC721, IERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
Basket.sol:24: _mint(_curator, 0);
17. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
18. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions
See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
NibblVault.sol:20:contract NibblVault is INibblVault, BancorFormula, ERC20Upgradeable, Twav, EIP712Base {
19. All initialize() functions are front-runnable in the solution
Consider adding some access control to them or deploying atomically or using constructor initializer:
Basket.sol:23: function initialise(address _curator) external override initializer {
NibblVault.sol:173: function initialize(
20. Use a constant instead of duplicating the same string
Basket.sol:23: function initialise(address _curator) external override initializer {
internall
Proxy/ProxyBasket.sol:26: * This function does not return to its internall call site, it will return directly to the external caller.
Proxy/ProxyVault.sol:26: * This function does not return to its internall call site, it will return directly to the external caller.
reenterancy
NibblVault.sol:125: ///@notice reenterancy guard
pausablity
NibblVault.sol:152: /// @dev pausablity implemented in factory
primaryReseveRatio
NibblVault.sol:200: //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator
NibblVault.sol:201: curatorFee = (((_secondaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO) * MIN_CURATOR_FEE) / (primaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO)) + MIN_CURATOR_FEE; //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator
seconday
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
Continous
NibblVault.sol:250: /// @dev The max continous tokens on SecondaryCurve is equal to initialTokenSupply
NibblVault.sol:270: /// @param _amount amount of reserve tokens to buy continous tokens
NibblVault.sol:282: /// @param _amount amount of reserve tokens to buy continous tokens
NibblVault.sol:359: /// @param _amtIn Continous Tokens to be sold
recieve
NibblVault.sol:361: /// @param _to Address to recieve the reserve token to
airdops
NibblVault.sol:512: /// @notice ERC20s can be accumulated by the underlying ERC721 in the vault as royalty or airdops
NibblVault.sol:531: /// @notice ERC1155s can be accumulated by the underlying ERC721 in the vault as royalty or airdops
2. Deprecated library used for Solidity >= 0.8 : SafeMath
NibblVaultFactory.sol:3:pragma solidity 0.8.10;
NibblVaultFactory.sol:9:import { SafeMath } from "@openzeppelin/contracts/utils/math/SafeMath.sol";"
From solidity version of at least 0.8.4 , you can use bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)
Table of Contents
whenNotPaused
is not used oncreateBasket
createBasket
usesbasketImplementation
for salt, butcreateVault
doesn't usevaultImplementation
for saltnibbledTokens
arrayAccessControlMechanism.sol
: propose/accept pattern is redundant sincegrantRole
can be used to push a roleTWAV
is only over four blockstransfer()
/transferFrom()
withIERC20
transfer()
/transferFrom()
not checkedreceive()
functioninitialize()
functions_safeMint()
should be used rather than_mint()
wherever possibleabi.encodePacked()
should not be used with dynamic types when passing the result to a hash function such askeccak256()
__gap[50]
storage variable to allow for new storage variables in later versionsinitialize()
functions are front-runnable in the solutionconstant
instead of duplicating the same string>= 0.8
: SafeMathLow Risk Issues
1.
whenNotPaused
is not used oncreateBasket
While
whenNotPaused
is used oncreateVault
:This seems to have been forgotten on
createBasket
:2.
createBasket
usesbasketImplementation
for salt, butcreateVault
doesn't usevaultImplementation
for salt3. Add constructor initializers
As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run
initialize
on an implementation contract, by adding an empty constructor with theinitializer
modifier. So the implementation contract gets initialized automatically upon deployment.”Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”
Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:
4. DDOS on
nibbledTokens
arrayThe minimum price to create a vault is pretty trivial, 1 gwei:
An attacker could spam and bloat the size of the
nibbledTokens
array to the pointgetVaults()
becomes unusable5. Check Effects Interactions pattern not respected
To avoid unexpected behavior in the future (be it for the solution or for a fork), it's recommended to always follow the CEI pattern.
Consider always moving the state-changes before the external calls.
Affected code:
6.
AccessControlMechanism.sol
: propose/accept pattern is redundant sincegrantRole
can be used to push a role7.
TWAV
is only over four blocksSomeone does not have to maintain price variation for long to reject a buyout. This can result in blocking of buyouts.
8. Unsafe casting may overflow
SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:
9. Basic ecrecover is used
Basic ecrecover is used, leading to being able to approve transfers from the zero address. Any tokens sent to 0 address can be recovered by another user.
Consider using OpenZeppelin’s ECDSA library
10. Missing address(0) checks
(Output from Slither) Consider adding an
address(0)
check here:11. Unsafe use of
transfer()
/transferFrom()
withIERC20
Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s
transfer()
andtransferFrom()
functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast toIERC20
, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’sSafeERC20
'ssafeTransfer()
/safeTransferFrom()
insteadBad:
Good (using OpenZeppelin's
SafeERC20
):Consider using
safeTransfer
/safeTransferFrom
here:12. Return values of
transfer()
/transferFrom()
not checkedNot all
IERC20
implementationsrevert()
when there's a failure intransfer()
/transferFrom()
. The function signature has aboolean
return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment.Bad:
Good (using
require
):Consider wrapping transfers in
require()
statements consistently here:Alternatively, SafeERC20 could be used here, as stated before
13. Unused/empty
receive()
functionIf the intention is for the Ether to be used, the function should call another function, otherwise it should revert
14. Lack of event emission after critical
initialize()
functionsSimilar issue in the past: here
To record the init parameters for off-chain monitoring and transparency reasons, please consider emitting an event after the
initialize()
functions:15. Prevent accidentally burning tokens
Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.
Consider adding a check to prevent accidentally burning tokens here:
16.
_safeMint()
should be used rather than_mint()
wherever possible_mint()
is discouraged in favor of_safeMint()
which ensures that the recipient is either an EOA or implementsIERC721Receiver
. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.17.
abi.encodePacked()
should not be used with dynamic types when passing the result to a hash function such askeccak256()
Similar issue in the past: here
Use
abi.encode()
instead which will pad items to 32 bytes, which will prevent hash collisions (e.g.abi.encodePacked(0x123,0x456)
=>0x123456
=>abi.encodePacked(0x1,0x23456)
, butabi.encode(0x123,0x456)
=>0x0...1230...456
). If there is only one argument toabi.encodePacked()
it can often be cast tobytes()
orbytes32()
instead.18. Upgradeable contract is missing a
__gap[50]
storage variable to allow for new storage variables in later versionsSee this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
19. All
initialize()
functions are front-runnable in the solutionConsider adding some access control to them or deploying atomically or using constructor
initializer
:20. Use a
constant
instead of duplicating the same string21. Calculation doesn't match with comment
Non-Critical Issues
1. Typos
2. Deprecated library used for Solidity
>= 0.8
: SafeMath3. Commented code
4. Missing friendly revert strings
5. Use a more recent version of solidity
From solidity version of at least 0.8.4 , you can use
bytes.concat()
instead ofabi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to getstring.concat()
instead ofabi.encodePacked(<str>,<str>)