Overly restrictive access control limits contract flexibility and collaboration potential.

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L23 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L61-L64 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L68-L76 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L131-L139

Vulnerability details

Impact

There are only two basic access control mechanisms:

• onlyOwner modifier for transferOwnership, addAttributeDivisor, and addAttributeProbabilities.
• deleteAttributeProbabilities also uses the onlyOwner modifier.
• While this limits changes to sensitive settings, consider more granular access control if you anticipate the contract interacting with other contracts or having a more complex permission system.

Overly Restrictive Permissions The AiArenaHelper contract employs a simplistic access control structure based solely on an _ownerAddress and the onlyOwner modifier. This means only the designated owner holds the power to make crucial changes to the following:

• Transferring Ownership: (transferOwnership)
• Modifying Attribute Divisors: (addAttributeDivisor): These values significantly impact the rarity calculations for a fighter's physical attributes.
• Managing Attribute Probabilities: (addAttributeProbabilities, deleteAttributeProbabilities): These directly influence the distribution of attributes across different generations of fighters.

The issue stems from a monolithic approach to authorization. All sensitive operations depend on a single address holding authority. This leads to several problems:

• Central Point of Failure: If the owner's private key is compromised, the entire integrity of the system is jeopardized. An attacker could manipulate probabilities, alter attribute calculations, or even transfer ownership to themselves.
• Hindrance to Collaboration: Complex game environments often require a more nuanced approach to permissions. You might want certain tasks (like tweaking attribute probabilities for balance) to be delegated to specific roles or other trusted contracts without handing over full ownership.

Proof of Concept

Imagine the AiArenaHelper contract is deployed on-chain, and the initial owner wants to hand over the fine-tuning of attribute probabilities to a game designer or balancer specializing in fighter creation. Currently, the owner would need to either:

• A. Risk Compromising Security: Transfer full ownership to the game designer. This jeopardizes the security of all other aspects of the contract.
• B. Become a Bottleneck: Remain the sole owner and be burdened by each request for probability updates. This hinders the game's development and responsiveness to balance changes.

src/AiArenaHelper.sol#L23

src/AiArenaHelper.sol#transferOwnership

src/AiArenaHelper.sol#addAttributeDivisor

src/AiArenaHelper.sol#addAttributeProbabilities

  address _ownerAddress;

  // ... other contract code ...

  function transferOwnership(address newOwnerAddress) external {
    require(msg.sender == _ownerAddress);
    _ownerAddress = newOwnerAddress;
  }

  function addAttributeDivisor(uint8[] memory attributeDivisors) external {
    require(msg.sender == _ownerAddress); 
    // ... code to set attribute divisors
  }

  function addAttributeProbabilities(uint256 generation, uint8[][] memory probabilities) public {
    require(msg.sender == _ownerAddress);
    // ... code to set attribute probabilities
  }  

As you can see, modifications to key functions in the contract are all guarded by the onlyOwner modifier.

Tools Used

VsCode

Recommended Mitigation Steps

Instead of a single owner address, require multiple authorized addresses to sign off on critical transactions (e.g., 3 out of 5 trusted addresses). This reduces the risk of a compromised single private key impacting the entire system.

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 #4

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)

[HickupHH3]

R

[c4-judge]

HickupHH3 marked the issue as grade-c