Nibbl contest - Limbooo's results

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L173 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L23 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L158 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L99

Vulnerability details

Impact

In OpenZeppelin Contracts (proxy/utils/Initializable.sol):

CAUTION: An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation...

Proof of Concept

This can lead to takeover of 2 contracts: Basket.sol and NibblVault.sol since implementation contracts not initialized and can be initialized publicly. https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol

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

Also, Upgrading either of their implementation in NibblVaultFactory.sol when proposeNewVaultImplementation(address _newVaultImplementation) or proposeNewBasketImplementation(address _newBasketImplementation) can lead to the same issue if the upgraded contract did not disable initializers. https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L158 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L99

Recommended Mitigation Steps

As its mentioned in OpenZeppelin Contracts documentation:

To prevent the implementation contract from being used, you should invoke the {_disableInitializers} function in the constructor to automatically lock it when it is deployed:

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

Should both of Basket.sol and NibbleVault.sol use the _disableInitializers(); which make the implementation contract unable to be initialized to version 1. Hence, for newer version of Basket.sol and NibbleVault.sol proposed for the factory should also be initialized to version 1 to prevent the attack https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L165 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L130

1- Custom Error

Impact

Custom Error is a convenient and gas-efficient way to revert errors. the revert("Error") uses more line of Yul and can lead to more gas usage.

Proof of Concept

All errors reverting using revert("Error").

Recommended Mitigation Steps

Define custom error, which can be used inside and outside of contracts (including interfaces and libraries).

error Unauthorized();

Revert the error

    if (msg.sender != owner)
        revert Unauthorized();

2- Use modifier for the same require

Impact

Modifier saved gas for the reusable code. In NibbleVault.sol contract:

require(msg.sender == curator,"NibblVault: Only Curator");

require(msg.sender == bidder, "NibblVault: Only winner");

Proof of Concept

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L475 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L486 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L496 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L505 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L524 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L536 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L546

Recommended Mitigation Steps

Define a modifier to apply the same logical error:

modifier onlyCurator(){
    require(msg.sender == curator,"NibblVault: Only Curator");
}

Also for onlyWinner(), Use the modifier for all function need these authorization.