Nibbl contest - Funen's results

1. Title : Used pause event for NibblVault

You can used this implementation for better use since it was no event and emitted inside contract

POC

https://docs.openzeppelin.com/contracts/2.x/api/lifecycle

Recommendation

Using event Paused(address account)

Emitted when the pause is triggered by a pauser (account).

event Unpaused(address account)

Emitted when the pause is lifted by a pauser (account).

2. Title : Use whenPaused() modifier

Modifier to make a function callable only when the contract is paused.

POC

https://docs.openzeppelin.com/contracts/2.x/api/lifecycle#Pausable-whenPaused--

This module is used through inheritance. It will make available the modifiers whenNotPaused and whenPaused, which can be applied to the functions of your contract. Note that they will not be pausable by simply including this module, only once the modifiers are put in place.

3. Title : Standart use of function initialize() was used so many times. So if necessary it can be changed from basket interface and basket.sol into intialize , you can removed another initialise and changed into initialize. Since it was good for readibility and structure.

Tool Used

Manual Review

4. Title : Avoid multiple initializations

https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVault.sol#L173

Make sure though that you do not allow multiple initializations. For just a few parameters, simply add a check for each parameter, For many parameters, add an isInitialized boolean state variable:

contract MyContract {
  bool isInitialized = false;

  function initialize(
    uint256 _param1,
    uint256 _param2,  
    uint256 _param3,
    address _param4,
    address _param5,
    bytes32 _param6,
    bytes32 _param7
  ) public {
    require(!isInitialized, 'Contract is already initialized!');
    isInitialized = true;

    param1 = _param1;
    ...
    param7 = _param7;
  }
}

Tool Used

Manual Review

5. Title : Avoid Floatin Pragma in AccessControlMechanism.sol

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Utilities/AccessControlMechanism.sol

Since it was used ^0.8.0. As the compiler can be use as 0.8.10 and consider locking at this version the same as another. It can be consider using locking the pragma version whenever possible and avoid using a floating pragma in the final deployment. Since it can be problematic, if there are publicly disclosed bugs and issues that affect the current compiler version used.

Tool Used

Manual Review

1. Title : Value can be caching in calldata instead of memory

File : EIP712Base.sol Line.15

    function INIT_EIP712(string memory name, string memory version) internal {

2. Title : Using ++i than i++ for saving more gas

Using i++ instead ++i for all the loops, the variable i is incremented using i++. It is known that implementation by using ++i costs less gas per iteration than i++.

Tools Used

Manual Review

Occurances

main/contracts/Basket.sol#L43        for (uint256 i = 0; i < _tokens.length; i++) {
main/contracts/Basket.sol#L70        for (uint256 i = 0; i < _tokens.length; i++) {
main/contracts/Basket.sol#L93        for (uint256 i = 0; i < _tokens.length; i++) {

2.) Title : change uint256 i = 0 into uint i for saving more gas

using this implementation can saving more gas for each loops.

Tool Used

Manual Review

Recommended Mitigation

Change it

Occurances

main/contracts/Basket.sol#L43        for (uint256 i = 0; i < _tokens.length; i++) {
main/contracts/Basket.sol#L70        for (uint256 i = 0; i < _tokens.length; i++) {
main/contracts/Basket.sol#L93        for (uint256 i = 0; i < _tokens.length; i++) {

4.) Title : Caching array length can saving more gas

This implementation can be saving more gas, since if caching the array length is more gas efficient. just because access to a local variable in solidity is more efficient.

Tool Used

Manual Review

Occurances

main/contracts/Basket.sol#L43        for (uint256 i = 0; i < _tokens.length; i++) {
main/contracts/Basket.sol#L70        for (uint256 i = 0; i < _tokens.length; i++) {
main/contracts/Basket.sol#L93        for (uint256 i = 0; i < _tokens.length; i++) {