Nibbl contest - Tadashi's results

Typos

• “reenterancy” at L125 of NibblVault.sol

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:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}

Impact: Code QA

Modifier lock should come before other modifiers

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.

Mitigation: Change L300 to

function buy(uint256 _minAmtOut, address _to) external override payable lock notBoughtOut whenNotPaused returns(uint256 _purchaseReturn) {

Impact: Code QA