`Neuron::mint()` - L156: `MAX_SUPPLY` represents a valid max value, but `require(totalSupply() + amount < MAX_SUPPLY` check implies that its not. Should use `<=` instead.

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/68be0262e285c32c126786eeb2915f2207019b15/src/Neuron.sol#L151-L159 https://github.com/code-423n4/2024-02-ai-arena/blob/68be0262e285c32c126786eeb2915f2207019b15/src/Neuron.sol#L156 https://github.com/code-423n4/2024-02-ai-arena/blob/68be0262e285c32c126786eeb2915f2207019b15/src/Neuron.sol#L42-L43

Vulnerability details

Impact

NOTE:

• Please ensure this bug report is seen by the protocol team.
• I think the bug is medium severity in terms of impact.
• This bug report is a tad long, apologies in advance, but please read/check everything carefully.

Neuron::mint() - L156: MAX_SUPPLY represents a valid max value, but require(totalSupply() + amount < MAX_SUPPLY require check implies that its not. Should use <= instead.

Attention:

• After checking out protocol test function Neuron.t.sol::testRevertMintWithMinterRoleExceedsMaxSupply() I came to the unfortunate conclusion that it seems there was confusion and/or inconsistency in protocol team's understanding of what the MAX_SUPPLY means. It is beyond any doubt crystal clear from current implementation and also common sense that MAX_SUPPLY is a valid value for the max supply for the token, i.e. we can mint up to and INCLUDING the max supply. For example, if max supply is 10k tokens, we can mint 10k tokens, but not more. Yet here it is incorrectly managed & implemented which means we can only mint max 10k - 1 tokens, as an example.

Summary: the bug I discovered isn't simply an oversight, it seems the dev team misunderstood the logic behind MAX_SUPPLY, and set up the test according to their incorrect understanding. In my PoC I point out the problems in the test function and I fix it and also fix the bug and run those tests too with the results below.

Affected function:

    function mint(address to, uint256 amount) public virtual {
        require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
        require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
        _mint(to, amount);
    }

Impact:

• will never be able to mint up to and including the max supply, will only ever be able to mint total of MAX_SUPPLY - 1 tokens.
• IF there was a critical functionality that depended on being able to mint the MAX_SUPPLY of tokens, that functionality would be DoS-ed.
• will DoS anyone wanting to mint the MAX_SUPPLY of tokens because they will know that they are not trying to mint more than the max supply the protocol allows, but the mint function still reverts with a confusing error message implying that they're trying to mint more than the max supply, but they are definitely not, so it will cause great confusion until a technical person/dev figures out the problem...
• not intended protocol behaviour, which is opposite of what users would expect.
• trust in protocol could be affected.

Proof of Concept

PoC:

Check the error message of the first require check in the mint function above. It clearly states ""Trying to mint more than the max supply"", i.e. MORE than the max supply, i.e. should only revert if > MAX_SUPPLY, and opposite of > is <=.

Also check the natspec comment for the declaration of the constant state variable, it clearly states maximum supply of NRN tokens:

    /// @notice Maximum supply of NRN tokens.
    uint256 public constant MAX_SUPPLY = 10**18 * 10**9;

And since we have function mint(address to, uint256 amount) public virtual {, it means that when trying to mint amount of tokens, to ensure we don't mint MORE than MAX_SUPPLY, we simply check that totalSupply() + amount <= MAX_SUPPLY.

Therefore < MAX_SUPPLY is incorrect and <= MAX_SUPPLY is correct.

The buggy test function. Max supply is still a valid value, we should be able to mint up to including max supply!:

    /// @notice Test owner with MINTER_ROLE minting max supply reverting. /// @audit-issue displays incorrect understanding
    function testRevertMintWithMinterRoleExceedsMaxSupply() public {
        uint256 maxSupply = _neuronContract.MAX_SUPPLY();
        address minter = vm.addr(3);
        _neuronContract.addMinter(minter);
        vm.prank(minter);
        vm.expectRevert("Trying to mint more than the max supply");
        _neuronContract.mint(minter, maxSupply); /// @audit here total supply should still be zero before the mint
    }

So they're knowingly treating the value of MAX_SUPPLY as an invalid max supply value, which demonstrates incorrect understanding by the team of the logic. In above test function the total supply is apparently 700000000000000000000000000 before the mint, and mint() parameter maxSupply is equal to MAX_SUPPLY, and they expect this function to revert. I think this part was oversight, because _neuronContract.totalSupply() should have been zero before calling mint(). Therefore I've had to make some modifications to account for the existing non-zero value of _neuronContract.totalSupply() so that the amount being minted + the existing total supply is exactly equal to the max supply. The _neuronContract.totalSupply() value reported by this test function before calling the mint() function is 700000000000000000000000000.

Example: I launch an NFT airdrop campaign, and in my minting smart contract I hardcode the max supply as 10k tokens. I say: uint256 public constant MAX_SUPPLY = 10_000; // Maximum supply of airdrop NFT tokens. This means I can mint maximum 10_000 NFTs during my airdrop campaign. My mint function's require check will be: require(totalSupply() + amount <= MAX_SUPPLY, "Trying to mint MORE than the max supply");

TESTS:

Using the modified test function, please take note of my audit tag comments below:

    function testRevertMintWithMinterRoleExceedsMaxSupply() public {
        uint256 maxSupply = _neuronContract.MAX_SUPPLY();
        uint256 neuronContractTotalSupply_before = _neuronContract.totalSupply(); /// @audit added for PoC/testing purposes
        address minter = vm.addr(3);
        _neuronContract.addMinter(minter);
        assertEq(neuronContractTotalSupply_before, 700000000000000000000000000); /// @audit added for PoC/testing purposes
        assertEq(_neuronContract.balanceOf(minter), 0); /// @audit added for PoC/testing purposes
        vm.prank(minter);
        vm.expectRevert("Trying to mint more than the max supply");
        _neuronContract.mint(minter, (maxSupply - neuronContractTotalSupply_before)); /// @audit added for PoC/testing purposes so that we take into account the existing 7M token totalSupply value, which we clear out before calling mint, so that we can test the mint() function correctly/properly, as this test function should have done with `totalSupply() = 0`
        //vm.expectRevert("Trying to mint more than the max supply"); /// @audit added for PoC/testing purposes
        //_neuronContract.mint(minter, maxSupply + 1); /// @audit added for PoC/testing purposes
        //assertEq(_neuronContract.balanceOf(minter), maxSupply); /// @audit added for PoC/testing purposes
        //uint256 neuronContractTotalSupply_after = _neuronContract.totalSupply(); /// @audit added for PoC/testing purposes
        //assertEq(neuronContractTotalSupply_after, maxSupply); /// @audit added for PoC/testing purposes
    }

Test results WITH the bug:

  [44374] NeuronTest::testRevertMintWithMinterRoleExceedsMaxSupply()
    ├─ [307] Neuron::MAX_SUPPLY() [staticcall]
    │   └─ ← 1000000000000000000000000000 [1e27]
    ├─ [2327] Neuron::totalSupply() [staticcall]
    │   └─ ← 700000000000000000000000000 [7e26]
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    ├─ [27181] Neuron::addMinter(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   ├─ emit RoleGranted(role: 0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6, account: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, sender: NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← ()
    ├─ [2586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   └─ ← 0
    ├─ [0] VM::prank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   └─ ← ()
    ├─ [0] VM::expectRevert(Trying to mint more than the max supply)
    │   └─ ← ()
    ├─ [776] Neuron::mint(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 300000000000000000000000000 [3e26])
    │   └─ ← revert: Trying to mint more than the max supply
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.31ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Test results with the BUG FIXED, using test function's default expect revert statement:

    ├─ [26258] Neuron::mint(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 300000000000000000000000000 [3e26])
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, value: 300000000000000000000000000 [3e26])
    │   └─ ← ()
    └─ ← call did not revert as expected

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.43ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/Neuron.t.sol:NeuronTest
[FAIL. Reason: call did not revert as expected] testRevertMintWithMinterRoleExceedsMaxSupply() (gas: 69858)

Encountered a total of 1 failing tests, 0 tests succeeded

Test results with the BUG FIXED, but commenting out test function's default expect revert statements and replacing with the appropriate ones as per below:

        vm.prank(minter);
        //vm.expectRevert("Trying to mint more than the max supply");
        _neuronContract.mint(minter, mintAmount);
        //vm.expectRevert("Trying to mint more than the max supply"); /// @audit added for PoC/testing purposes
        //_neuronContract.mint(minter, mintAmount + 1); /// @audit added for PoC/testing purposes
        assertEq(_neuronContract.balanceOf(minter), mintAmount); /// @audit added for PoC/testing purposes
        uint256 neuronContractTotalSupply_after = _neuronContract.totalSupply(); /// @audit added for PoC/testing purposes
        assertEq(neuronContractTotalSupply_after, maxSupply); /// @audit added for PoC/testing purposes

Test results:

  [71403] NeuronTest::testRevertMintWithMinterRoleExceedsMaxSupply()
    ├─ [307] Neuron::MAX_SUPPLY() [staticcall]
    │   └─ ← 1000000000000000000000000000 [1e27]
    ├─ [2327] Neuron::totalSupply() [staticcall]
    │   └─ ← 700000000000000000000000000 [7e26]
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    ├─ [27181] Neuron::addMinter(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   ├─ emit RoleGranted(role: 0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6, account: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, sender: NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← ()
    ├─ [2586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   └─ ← 0
    ├─ [0] VM::prank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   └─ ← ()
    ├─ [26258] Neuron::mint(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 300000000000000000000000000 [3e26])
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, value: 300000000000000000000000000 [3e26])
    │   └─ ← ()
    ├─ [586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   └─ ← 300000000000000000000000000 [3e26]
    ├─ [327] Neuron::totalSupply() [staticcall]
    │   └─ ← 1000000000000000000000000000 [1e27]
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.52ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

And finally the test results with the BUG FIXED but now testing for trying to mint MORE than the max supply value. It should revert, again with the relevant test function modifications:

        vm.prank(minter);
        //vm.expectRevert("Trying to mint more than the max supply");
        //_neuronContract.mint(minter, mintAmount);
        vm.expectRevert("Trying to mint more than the max supply"); /// @audit added for PoC/testing purposes
        _neuronContract.mint(minter, mintAmount + 1); /// @audit added for PoC/testing purposes
        //assertEq(_neuronContract.balanceOf(minter), mintAmount); /// @audit added for PoC/testing purposes
        assertEq(_neuronContract.balanceOf(minter), 0); /// @audit added for PoC/testing purposes
        uint256 neuronContractTotalSupply_after = _neuronContract.totalSupply(); /// @audit added for PoC/testing purposes
        //assertEq(neuronContractTotalSupply_after, maxSupply); /// @audit added for PoC/testing purposes
        assertEq(neuronContractTotalSupply_after, neuronContractTotalSupply_before); /// @audit added for PoC/testing purposes

Test results:

  [46435] NeuronTest::testRevertMintWithMinterRoleExceedsMaxSupply()
    ├─ [307] Neuron::MAX_SUPPLY() [staticcall]
    │   └─ ← 1000000000000000000000000000 [1e27]
    ├─ [2327] Neuron::totalSupply() [staticcall]
    │   └─ ← 700000000000000000000000000 [7e26]
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    ├─ [27181] Neuron::addMinter(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   ├─ emit RoleGranted(role: 0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6, account: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, sender: NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← ()
    ├─ [2586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   └─ ← 0
    ├─ [0] VM::prank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   └─ ← ()
    ├─ [0] VM::expectRevert(Trying to mint more than the max supply)
    │   └─ ← ()
    ├─ [779] Neuron::mint(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 300000000000000000000000001 [3e26])
    │   └─ ← revert: Trying to mint more than the max supply
    ├─ [586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   └─ ← 0
    ├─ [327] Neuron::totalSupply() [staticcall]
    │   └─ ← 700000000000000000000000000 [7e26]
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.64ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

VSCodium. Manual.

Recommended Mitigation Steps

Recommendations:

Besides fixing the mint() function please also fix the test function testRevertMintWithMinterRoleExceedsMaxSupply().

Mint function:

    function mint(address to, uint256 amount) public virtual {
-       require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
+       require(totalSupply() + amount <= MAX_SUPPLY, "Trying to mint MORE than the max supply");
        require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
        _mint(to, amount);
    }

Test function:

-   /// @notice Test owner with MINTER_ROLE minting max supply reverting.
+   /// @notice Test owner with MINTER_ROLE minting MORE than max supply reverting.
    function testRevertMintWithMinterRoleExceedsMaxSupply() public {
+       /// ensure totalSupply() is zero before calling mint() below, or else account for this as I did in my PoC tests
        uint256 maxSupply = _neuronContract.MAX_SUPPLY();
        address minter = vm.addr(3);
        _neuronContract.addMinter(minter);
        vm.prank(minter);
        vm.expectRevert("Trying to mint more than the max supply");
-       _neuronContract.mint(minter, maxSupply);
+       _neuronContract.mint(minter, maxSupply + 1);
    }

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #7

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)

[HickupHH3]

R

1318: L

[c4-judge]

HickupHH3 marked the issue as grade-c