AI Arena - SovaSlava's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L291-L303

Vulnerability details

Impact

User can transfer nft gameItem, even it has transferable property with value False.

Proof of Concept

In contract GameItems there is function safeTransferFrom, which override function with the same name from ERC1155 contract and check restriction of transferable.

But standart contract from OZ ERC1155 has another function, which allow transfer tokens - safeBatchTransferFrom, and this function has not overrided by safeTransferFrom from GameItems contract.

So. for transfer nft, which restricted for transfer, user could just call GameItemsContract.safeBatchTransferFrom(userAddress,ReceiverAddress, nftid, amount, "");
Where nftid is array with nft id and amount is also array.

    function testTransfer() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 2); //paying 2 $NRN for 2 batteries
        _gameItemsContract.adjustTransferability(0, false);
        uint[] memory  nftid = new uint[](1);
        nftid[0] = 0;
        uint[] memory amount = new uint[](1);
        amount[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(address(this),address(1), nftid, amount, "");
    }

Tools Used

Manual review

Recommended Mitigation Steps

Also override safeBatchTransferFrom function in GameItems contract

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L93-L112

Vulnerability details

Impact

The owner can only issue a role, there is no mechanism to revoke the role from the user

Proof of Concept

Contract have several functions, which allow owner grant roles to users (addMinter, addStaker, addSpender). But owner could not revoke these roles, because he is not have DEAULT_ADMIN_ROLE, which usually grants to owner in constructor. So, if owner try to call function from OZ Access contract revokeRole(), it will revert, because owner dont have role 0x0, which is admin of all roles by default

OZ docs - https://docs.openzeppelin.com/contracts/3.x/access-control#:~:text=This%20mechanism%20can%20be%20used%20to%20create%20complex%20permissioning%20structures%20resembling%20organizational%20charts%2C%20but%20it%20also%20provides%20an%20easy%20way%20to%20manage%20simpler%20applications.%20AccessControl%20includes%20a%20special%20role%2C%20called%20DEFAULT_ADMIN_ROLE%2C%20which%20acts%20as%20the%20default%20admin%20role%20for%20all%20roles.

POC: // add this code to test/Neuron.t.sol

    function testRevokeRole() public {
        address minter2 = makeAddr("secondMinter");
        _neuronContract.addMinter(minter2);
// will revert
        _neuronContract.revokeRole(_neuronContract.MINTER_ROLE(),minter2);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Add in constructor this line

_setupRole(DEFAULT_ADMIN_ROLE, msg.sender);

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L292-L299

Vulnerability details

Impact

User could not claim tokens, if current round is big number, and before, user has not never call claim function.

Proof of Concept

if the user has never called the claimNRN() function and the current round is, for example, 1000, then the loop will go through 1000 times and calculate the user's reward for each loop. This will result in high gas consumption and cause the transaction to revert

        uint32 lowerBound = numRoundsClaimed[msg.sender]; // <----- 0
    
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            nrnDistribution = getNrnDistribution(currentRound);
            ...

Tools Used

Manual review

Recommended Mitigation Steps

Create partial passage through the loop - pagination mechanism in claimNRN function, so user can specify from which round he would like iterate. But startRound should be greater than numRoundsClaimed[msg.sender]

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L185-L188 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L139-L142

Vulnerability details

Impact

Owner could not disable old burning address

Proof of Concept

Contract has function, which grant privilege to burn tokens from other addresses. its function - setAllowedBurningAddresses. But if owner decide to change address, he could only grant privilege for new address, but he can not deprive privilege old address, because there is not such function, which set value False in mapping allowedBurningAddresses.

The same problem in FighterFarm.sol with staker role

    function addStaker(address newStaker) external {
        require(msg.sender == _ownerAddress);
        hasStakerRole[newStaker] = true;
        
    }

Contract dont have function for remove staker

Tools Used

Manual review

Recommended Mitigation Steps

Add new function

    function delAllowedBurningAddresses(address newBurningAddress) public {
        require(isAdmin[msg.sender]);
        allowedBurningAddresses[newBurningAddress] = false;
       
    }

Assessed type

Access Control