Details: In Basket.sol the initializer function is spelt as initialise, but the other contracts use the variant initialize. Consider renaming initialise to initialize to avoid confusion in the future.
Mitigation: Rename initialise to initialize in Basket.sol and corresponding test files.
Impact: Code QA
Initializing the implementation contract
Details: Not initializing a contract after deployment could result in exploits, e.g. GHSA-5vp3-v4hc-gx76 (more details here). While the contracts use a recent version of OpenZeppelin that mitigates this particular issue, it is still recommend to ensure that (even implementation) contracts cannot be initialized after deployment.
Mitigation: Following OpenZeppelin docs, add the following function to the implementation contracts:
Details: At L300 of NibblVault.sol the modifier lock should be the first modifier in order to prevent execution of the other modifiers in case of reentrancy. While currently there is no obvious vulnerability, it is a good practice to place it in the first position.
Typos
Consistent usage for function names
Details: In Basket.sol the initializer function is spelt as initialise, but the other contracts use the variant initialize. Consider renaming initialise to initialize to avoid confusion in the future.
Mitigation: Rename initialise to initialize in Basket.sol and corresponding test files.
Impact: Code QA
Initializing the implementation contract
Details: Not initializing a contract after deployment could result in exploits, e.g. GHSA-5vp3-v4hc-gx76 (more details here). While the contracts use a recent version of OpenZeppelin that mitigates this particular issue, it is still recommend to ensure that (even implementation) contracts cannot be initialized after deployment.
Mitigation: Following OpenZeppelin docs, add the following function to the implementation contracts:
Impact: Code QA
Modifier
lock
should come before other modifiersDetails: At L300 of NibblVault.sol the modifier
lock
should be the first modifier in order to prevent execution of the other modifiers in case of reentrancy. While currently there is no obvious vulnerability, it is a good practice to place it in the first position.Mitigation: Change L300 to
Impact: Code QA