Unchecked constructor arguments can make a contract unworkable

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L132,https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L109,https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerNXT.sol#L27,https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerRNG.sol#L32,https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L44,https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L39

Vulnerability details

Impact

In the NextGenCore contract constructor, there is no check that a valid admin contract is set using NextGenAdmins::isAdminContract().

If the contract address in the _adminsContract constructor is set incorrectly, it is not possible to call admin functions in the contract, which will cause the contract to become inoperable.

It is impossible to change the address of the contract after deployment, because NextGenCore::updateAdminContract() uses the FunctionAdminRequired() modifier, which calls the current admin contract. But since we have it specified incorrectly, it is impossible to access its functions.

This leads to the problem that if you initially set an incorrect address, it is impossible to change it later. The functions cannot be performed from the admin role.

This problem is found in contracts:

• NextGenCore.sol
• RandomizerVRF.sol
• MinterContract.sol
• NextGenRandomizerNXT
• AuctionDemo.sol
• NextGenRandomizerRNG.sol

Proof of Concept

Deployer specifies the wrong contract address in the constructor argument _adminsContract, which is not INextGenAdmins.

After the deployment, it tries to call a function

function updateAdminContract(address _newadminsContract) public FunctionAdminRequired(this.updateAdminContract.selector) {}

but triggers the FunctionAdminRequired() modifier, which checks for access to that function with adminsContract set:

modifier FunctionAdminRequired(bytes4 _selector) {
    require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true , "Not allowed");
    _;
}

Of course FunctionAdminRequired returns an error because it cannot call the adminsContract contract functions due to a non-existent address.

To run the test, change /hardhat/scripts/fixturesDeployment.js to a random address (line 26)

const hhCore = await nextGenCore.deploy(
    "Next Gen Core",
    "NEXTGEN",
-    await hhAdmin.getAddress(),
+    '0x1231233300000000000000000000000000000001'
)

Add this code to /hardhat/nextGen.test.js and run test (npx hardhat test --grep "Change admin")

context("Change admin contract after deploy", () => {    
        it("Call updateAdminContract()", async function () {
            await contracts.hhCore.updateAdminContract(
                contracts.hhAdmin.getAddress() // _adminsContract
            )
        })
})

Tools Used

Manual analysis, Hardhat, VS Code

Recommended Mitigation Steps

Check values in the constructor

require(INextGenAdmins(_adminsContract).isAdminContract() == true, "Contract is not Admin")

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[141345]

bot race known issue L-02

[alex-ppg]

The Warden's submission entails validation of input addresses in constructors that is an exact match of bot issue L-23.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope