AI Arena - peanuts's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L545 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L337-L366

Vulnerability details

Impact

Users can own more than MAX_FIGHTERS_ALLOWED.

Proof of Concept

In FighterFarm.sol, the transferFrom() and safeTransferFrom() functions are overridden to call _ableToTransfer()

    function transferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
 >      require(_ableToTransfer(tokenId, to));
        _transfer(from, to, tokenId);
    }


    /// @notice Safely transfers an NFT from one address to another.
    /// @dev Add a custom check for an ability to transfer the fighter.
    /// @param from Address of the current owner.
    /// @param to Address of the new owner.
    /// @param tokenId ID of the fighter being transferred.
    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
>       require(_ableToTransfer(tokenId, to));
        _safeTransfer(from, to, tokenId, "");
    }

_ableToTransfer() checks that the balanceOf() the user is not greater than MAX_FIGHTERS_ALLOWED.

    function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
>         balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId]
        );
    }

The contract inherits ERC721.

In ERC721, there are actually 2 safeTransferFrom() functions that has a public visibility.

>    function safeTransferFrom(address from, address to, uint256 tokenId) public virtual override {
        safeTransferFrom(from, to, tokenId, "");
    }


    /**
     * @dev See {IERC721-safeTransferFrom}.
     */
>    function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual override {
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner or approved");
        _safeTransfer(from, to, tokenId, data);
    }

Note that one has 3 parameters and one has 4 parameters. The user can call the safeTransferFrom() function with 4 parameters directly and transfer the tokens to bypass MAX_FIGHTERS_ALLOWED.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend overriding the other safeTransferFrom() function as well. Best is to put the _ableToTransfer() function in _beforeTokenTransfer() and _afterTokenTransfer().

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Users can participate in the points system and potentially get NRN distribution without any consequences.

Proof of Concept

To get some points, users have to stake some tokens. These tokens are succeptible to loss if they lose the battle. However, users can bypass the stakeAtRisk functionality by depositing 1 wei of token. This way, users can still earn points from their fighter but will not lose anything tokens.

1. Users deposit 1 wei as their stake. Their staking factor will be 1.
2. Since their amountStaked() is more than 0, _addResultPoints() will be called.
3. If their amount staked is only 1 wei, the curStakeAtRisk will be truncated to zero.

        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        curStakeAtRisk = (10 * 1) / 10**4 = 0;

If they lose the battle, they will not lose any tokens. If they win the battle, they will be able to get points since their stakingFactor is 1.

            /// If the user has no NRNs at risk, then they can earn points
            if (stakeAtRisk == 0) {
                points = stakingFactor[tokenId] * eloFactor;
            }

Users can earn some points from their fighters without any negative consequence.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend making sure that curStakeAtRisk is always above zero.

Assessed type

Context

Judge has assessed an item in Issue #1898 as 3 risk. The relevant finding follows:

[L-07] User can still earn points without staking NRNs. Document notes:

If a player does not stake NRNs, their NFT will not accrue Points or NRN rewards (i.e. Staking Factor would be 0 and the product of the equation is 0).

It is possible that a player accrue Points without staking any NRNs. This issue assumes that stake NRNs means the process of staking NRNs only, and not having NRNs that are at risk.

function _getStakingFactor(
    uint256 tokenId, 
    uint256 stakeAtRisk
) 
    private 
    view 
    returns (uint256) 
{
  uint256 stakingFactor_ = FixedPointMathLib.sqrt(

    (amountStaked[tokenId] + stakeAtRisk) / 10**18

  );

if (stakingFactor_ == 0) {
  stakingFactor_ = 1;

  }

  return stakingFactor_;
}    

Since _getStakingFactor() takes stakeAtRisk as well, and rounds up the stakingFactor to 1 should it be zero, users can simply get a fighter with some stakeAtRisk tokens, and withdraw all their current amountStaked tokens.

This way, the user will not need to stake anything but will be able to earn points from their fighter.

Make sure _addResultPoints() is called only if amountStaked[tokenId] > 0, and not amountStaked[tokenId] + stakeAtRisk > 0

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L146-L160

Vulnerability details

Impact

Winner cannot claim their NFTs.

Proof of Concept

Every round, a winner is selected in the MergingPool through pickWinner(). The admin will run the pickWinner() function which will set the winnerAddress[roundId] to the currentWinnerAddress. Note that currentWinnerAddress is a newly created array of address which logs all the winners.

    function pickWinner(uint256[] calldata winners) external {
        require(isAdmin[msg.sender]);
        require(winners.length == winnersPerPeriod, "Incorrect number of winners");
        require(!isSelectionComplete[roundId], "Winners are already selected");
        uint256 winnersLength = winners.length;
>       address[] memory currentWinnerAddresses = new address[](winnersLength);
        for (uint256 i = 0; i < winnersLength; i++) {
            currentWinnerAddresses[i] = _fighterFarmInstance.ownerOf(winners[i]);
            totalPoints -= fighterPoints[winners[i]];
            fighterPoints[winners[i]] = 0;
        }
>       winnerAddresses[roundId] = currentWinnerAddresses;
        isSelectionComplete[roundId] = true;
        roundId += 1;
    }

The winner will then call claimRewards(). The function will loop through all the winnerAddress of each round and mints the winner an NFT.

If a winner forgets to claim his NFT, and if a long time had passed and the winner claims again, he will be unable to claim his two NFTs because of out of gas errors.

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (msg.sender == winnerAddresses[currentRound][j]) {
                    _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                    claimIndex += 1;
                }

Let's say Alice is the winner in round 1. She forgets to claim her NFT and 100 round passes. Alice finally remembers that she plays this game and tries to get another NFT. She is also a winner of round 101.

When she tries to claim her reward, the lowerBound is 1 and the roundId is at 102.

The function will then loop through every single winner from every round until it reaches round 101.

By that time, the function will reach out of gas error and Alice will be unable to claim her two NFTs.

Tools Used

Recommended Mitigation Steps

Instead of doing a nested loop, consider creating a mapping with the roundId and the winners so that there is no need to update the numRoundsClaimed.

Assessed type

Loop

Lines of code

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

Vulnerability details

Impact

Later users may not be able to claim their NRN due to out of gas errors

Proof of Concept

If a user wins a battle, he is entitled to claim some NRN via the claimNRN() function. Here is how the function looks like. Note the loop, lowerBound and roundId

    function claimNRN() external {
        require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");
        uint256 claimableNRN = 0;
        uint256 nrnDistribution;
>       uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            nrnDistribution = getNrnDistribution(currentRound);
            claimableNRN += (
                accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
            ) / totalAccumulatedPoints[currentRound];
            numRoundsClaimed[msg.sender] += 1;
        }
        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

The function loops from the currentBound to the current roundId. The current bound is selected from the lowerBoundwhich is fromnumRoundsClaimed[msg.sender]`.

        uint32 lowerBound = numRoundsClaimed[msg.sender];

Let's say the protocol has been around for many years, and the roundId is at ~1000. If a user participates in the protocol at that stage, he has to go through 1000 rounds of loops in order to claim his NRN. This may result in out of gas errors.

Tools Used

Manual Review

Recommended Mitigation Steps

Have a nested mapping to allow the msg.sender to claim tokens from the rounds that he has won instead of looping each round.

Assessed type

Loop

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L370-L379

Vulnerability details

Impact

Users can choose not to reRoll since they already can predict the stats of the new fighter, which games the system.

Proof of Concept

A user can call reRoll() to rolls a new fighter with random traits. Note how dna is calculated.

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");


        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
>           numRerolls[tokenId] += 1;
>           uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;

_createFighterBase() will get the element, weight and newDna.

    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
        uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

When rerolling, the dna variable will always change because of numRerolls[tokenId] += 1. However, the user can predict his traits by calculating the dna off chain since only numRerolls is different.

Thus, the reRolls isn't really random. A user can choose not to reroll. Also, a user can calculate how many tries it will take to get a decent element and weight.

Tools Used

Manual Review

Recommended Mitigation Steps

Induce some true randomness into the dna calculation by using Chainlink VRF.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Users can game the system by getting back their stake at risk tokens without any repurcussions.

Proof of Concept

A user first stake some tokens into the RankedBattle contract through stakeNRN(). If the user loses a battle and he does not have any points, some of his staked tokens will be transferred to the stakeAtRisk contract.

            } else {
                /// If the fighter does not have any points for this round, NRNs become at risk of being lost
>               bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }

The moment the user has some tokens in the stakeAtRisk contract, he can withdraw all his tokens in the RankedBattle contract, leaving amountStaked[tokenId] as zero. He then continues participating in battles.

If the user loses the battle, the curStakeAtRisk will be zero since amountStaked[tokenId] will be zero. He will not lose any tokens.

            /// Do not allow users to lose more NRNs than they have in their staking pool
            if (curStakeAtRisk > amountStaked[tokenId]) {
                curStakeAtRisk = amountStaked[tokenId];
            }

If the user wins the battle, the curStakeAtRisk will be calculated as such. For example, assume bpsLostPerLoss is 10% (1000) and stakeAtRisk is 1000e18 tokens.

        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        curStakeAtRisk = 1000 * (0 +1000e18) / 10**4 = 100e18

If the user wins the battle, he gets to claim the stakeAtRisk tokens

            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
            }

This way, a user that has stakeAtRisk tokens can just withdraw all his staked tokens and simply continue battling. He will not face any negative consequences.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider punishing the user if he has no staked tokens and has stakeAtRisk tokens. One punishment can be burning a percentage of the stakeAtRisk so that the user can never get it back anymore.

Assessed type

Math

Judge has assessed an item in Issue #1898 as 2 risk. The relevant finding follows:

[L-08] A fighter with stakeAtRisk tokens cannot redeem their tokens if the owner is changed

Summary

• Users can get their fighters, which are ERC721 tokens, through a mint pass or by winning ranked battles

• These fighters have unique traits and abilities which can be rerolled

• These fighters will be used in ranked battles. In every battle, points can be earned or deducted

• These fighters can be trained to be stronger. If they are stronger, they can battle in a higher elo pool, which earns more points

• The ranked battle is a one on one battle. The fighters will be matched against one of similar elo.

• A user has to stake some tokens in order to earn points. If the user wins a battle, he gets some points. If the user loses the battle, he loses some points

• If the user has no points to lose, then his staked tokens will be at risk. The user can claim back the tokens if he wins a battle.

• There can be many battles in a round. Once the round is over, at risk tokens will be sent to the treasury

• Users can earn NRN tokens from the points that they accumulated. Every battle has a set number of NRN tokens to be distributed proporitionally to the number of points earned

• Every battle uses 10 voltage, which is recharged to 100 everyday. Only the challenger (the person who clicks battle) will use 10 voltage. If the user decides to play more than 10 battles in a day (using more than 100 voltage), he has the option to buy and use a battery, which will recharge the voltage back to 100

• The protocol also has an ERC1155 token shop where the user can buy stuff to help with their battles (one of the stuff is the battery)

Codebase quality analysis

GameItems.sol

• controls the minting and allowance of every user
• Every ERC1155 item has a finite or infinite supply. Every user has a daily allowance. A user can only buy x number of ERC1155 item a day.

Checks

• Checked user unable to mint when price is 0 (game item not set)
• Checked allowance is all computed properly. If token is finite, when bought, the supply is decreased properly.
• Checked daily allowance replenish, and user cannot game the system
• Checked Neuron is actually paid to the Neuron contract

MergingPool.sol

• This contract focuses on getting the winners for each round and giving them an NFT

• All function has proper access controls

• ClaimRewards cannot be gamed as it is a protected function

Potential Issues

• If the winner doesn't claim and the rounds continue, he might get an out of gas error.
• updateWinnersPerPeriod has no upper or lower bound
• The roundID is not synced to the Ranked Battle roundID. The points may not work as intended

Neuron.sol

• 1 billion total supply of NRN (Neuron)
• 700 million is minted at constructor, 200 million goes to the treasury and 500 million goes to the contributor.
• A classic ERC20 token contract with additional functions. Take note of setupAirdrop because there is no limit, a malicious admin can airdrop the whole treasury amount to a recipient.

Checks

• All important functions have the correct access control
• All burn functions cannot be called by anyone except approved address or self.
• Checked that mint amount cannot be more than max supply

Potential Issues

• No check for address0, no two step ownership transfer. (Low)
• Treasury approval to recipient can be dangerous. Have to look deeper to find an impact.

RankedBattle.sol

• transferOwnership(), adjustAdminAccess(), setGameServerAddress(), setStakeAtRiskAddress(), instantiateNeuronContract(), instantiateMergingPoolContract() functions all rely on the ownerAddress and changes the address accordingly

• Main part of the whole protocol, users must stake some tokens in order to earn points. UpdateBattleRecord is called by GameServer, which is an unknown entity.
• Points are used to distribute NRN every round.
• Users have the risk of losing NRN tokens if they lose their battles. They are able to claim back the NRN tokens if they win their battles before the round is over.

Checks

• Checked all sensitive functions have access control
• Checked all integration with other contracts works well
• Checked fighters cannot be transferred if there are staked tokens
• Checked that the 5 outcomes of the ranked battle is coded properly

1) Win + no stake-at-risk = Increase points balance
2) Win + stake-at-risk = Reclaim some of the stake that is at risk
3) Lose + positive point balance = Deduct from the point balance
4) Lose + no points = Move some of their NRN staked to the Stake At Risk contract
5) Tie = no consequence

• Checked that the NRN is deposited in to the correct contracts

StakeAtRisk.sol

• A relatively small contract that interacts with RankedBattle.sol
• Most important variable is the stakeAtRisk. If the round is changed, the stakeAtRisk tokens will be swept to the treasury address.

Checks

• Checked all functions have appropriate access control
• Checked interaction with RankedBattle is correct
• Checked all variables are increased and decreased properly
• Checked transfer of neuron is correct

Potential Issues

• NIL

VoltageManager.sol

• A contract that focuses on recharging and usage of batteries. I believe one match uses 10 voltage, and if all the voltage is used, the player can either wait for the replenish time (1 day) or use a battery.
• Battery can be used even at 99 voltage or 0 voltage, to reset the player's voltage to 100.

Checks

• All functions work as intended. All access control is correct.
• Battery and replenish works as intended.
• External intergration to use game item and burn is correct, although tokenID must be 0.

Potential Issues

• Lack of zero address check (Low)
• Lack of two step ownership transfer (Low)
• ERC1155 battery MUST have tokenID == 0. (The GameItems contract doesn't enforce this) (Low)

Centralization Risks

• In Neuron.sol, the MINTER_ROLE can mint as much tokens as he wants, up to total supply. This MINTER_ROLE is set by the _ownerAddress().
• In Neuron.sol, the admin can set up any amount of airdrops to any amount of recipient
• In RankedBattle.sol, the owner controls all the contract addresses.
• In RankedBattle.sol, the admin can call setNewRound() any time if the totalAccumulatedPoints > 0.
• In RankedBattle.sol, the admin can set the bps lost to > 100%
• In RankedBattle.sol, the admin controls the rankedNrnDistribution
• In GameItems.sol, the admin is incharge of setting the price of the game items in createGameItem()
• In GameItems.sol, the admin can burn all the ERC1155 tokens of any users
• In MergingPool.sol, the admin can set the winners of NFTs through pickWinner()
• In MergingPool.sol, the admin sets the number of winners per period, and can set to any amount (including zero)

Systemic Risks

• At some point in time, the MAX_SUPPLY will be reached. All battles will not be incentivzed anymore
• It is unknown how winners are picked in MergingPool and how many winners are picked each time.
• The rounds can be changed at any time in RankedBattle.setNewRound(). There is no fixed schedule

Architecture Review

Design Patterns and Best Practices:

• Modifiers should be used if code is repeated.
• Code does not adhere to CEI interaction when transferring tokens, although this does not cause any major issues

Code Readability and Maintainability:

• Code is pretty manageable and all external calls to different contract is well connected.
• Code does not use any external intergration except OpenZeppellin, which makes it easier to comprehend
• Code is written cleanly. Function names are easy to understand. There is no complex math algorithm or usage of assembly.
• Some parts of the code is not explained well enough (like how eloFactor is calculated, how is a winner chosen in MergingPool)

Error Handling and Input Validation:

• Events and Error messages are easy to understand.
• Input is not validated. Important functions lack a lower and upper bound, and functions take takes in an address change lacks a zero address check.

Interoperability and Standards Compliance:

• Good knowledge of the ERC20 and ERC1155 standard. Creation of ERC1155 tokens is properly done
• ERC721 functions are overridden, but MAX_FIGHTERS_ALLOWED can still be bypassed by calling safeTransferFrom() in the original ERC721 contract (with 4 parameters)

Testing and Documentation:

• Extensive tests (unit, integration tests) done well.
• Documentation (whitepaper, github) is plentiful and includes quality reasoning at every juncture of the protocol.
• The AI training part and its relation to Solidity requires more explanation (although I think all those are done off chain in other servers)

Upgradeability:

• Protocol is not intended to be upgradeable, but contracts can be redeployed and rerouted easily through all the onlyOwner functions.

Dependency Management:

Protocol relies on external libraries like OpenZeppelin. Protocol should keep an eye on vulnerabilities that affects those external integrations, and make changes where necessary.

Overall, great architecture from the protocol, slight changes would be to the written code itself (using modifiers for repeated code, checking zero values, checking overflows/underflows etc).

Time spent:

20 hours