Neuron.sol admin is eligible to transfer NRN tokens from any account

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L171-L177 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L184-L190 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L98-L112

Vulnerability details

Impact

The admin of Neuron.sol has the capability to transfer NRN tokens from any account, presenting a potential risk to users holding NRN tokens.

Bug Description

Neuron.sol features an admin who can assign STAKER_ROLE and SPENDER_ROLE to specific addresses. Holders of these roles can autonomously set allowances for any account to themselves, effectively granting them the authority to spend tokens from any account.

Despite the trust placed in the owner of the Neuron.sol contract, granting such extensive powers poses a potential risk to token holders, particularly if the token is also traded on secondary markets. The significant authority embedded in an ERC20 token like this should be addressed to mitigate potential risks.

    function approveSpender(address account, uint256 amount) public {
        require(
            hasRole(SPENDER_ROLE, msg.sender), 
            "ERC20: must have spender role to approve spending"
        );
        _approve(account, msg.sender, amount);
    }

    function approveStaker(address owner, address spender, uint256 amount) public {
        require(
            hasRole(STAKER_ROLE, msg.sender), 
            "ERC20: must have staker role to approve staking"
        );
        _approve(owner, spender, amount);
    }

Proof of Concept

N/A

Tools Used

Foundary.

Recommended Mitigation Steps

There are two proposed solutions to mitigate the risk:

1. Hardcode the contracts for SPENDER_ROLE/STAKER_ROLE and prevent admins from assigning new addresses to these roles.
2. Eliminate the SPENDER_ROLE/STAKER_ROLE designations entirely, and instead require users to manually approve NRN token allowances to the relevant contracts during interactions.

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #20

[c4-judge]

HickupHH3 marked the issue as not a duplicate

[HickupHH3]

recommendation, but does not identify the vuln of not being able to revoke roles

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)

[HickupHH3]

1R

[c4-judge]

HickupHH3 marked the issue as grade-c