AI Arena - DMoore's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L544 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L355-L365 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L348

Vulnerability details

Impact

FighterFarm derives from ERC721 and override transferFrom and safeTransferFrom function, which ensure that FighterFarm can only be transferred if _ableToTransfer returns true.

However, below safeTransferFrom is not override in FighterFarm to add this check.

    /**
     * @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 nor approved");
        _safeTransfer(from, to, tokenId, data);
    }

So that malicious user can use this function to transfer FighterFarm even if _ableToTransfer returns false, which means that staked FighterFarm can be transferred and malicious can own more than MAX_FIGHTERS_ALLOWED FighterFarm.

Proof of Concept

malicious user can use safeTransferFrom to transfer FighterFarm with _ableToTransfer returns false, see below code:

staked FighterFarm can be transferred.

    function testTransferringFighter() 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);
        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        // we can bypass check
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0) == _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
    }

User can own more than MAX_FIGHTERS_ALLOWED FighterFarm

    function testTransferringFighter() public {
        _mintFromMergingPool(_ownerAddress);
        for(uint256 i = 0; i < 10; i++) {
            _mintFromMergingPool(_DELEGATED_ADDRESS);
        }
        // check that unable to transfer since reaches 10
        vm.expectRevert();
        _fighterFarmContract.transferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        // we can bypass check
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0) == _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS), 11);
    }

Tools Used

Manual Review and Foundry

Recommended Mitigation Steps

Override function safeTransferFrom( address from, address to, uint256 tokenId, bytes memory data ) to add _ableToTransfer check

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

GameItems derives from ERC1155 and override safeTransferFrom function, which ensure that GameItems can only be transfered if allGameItemAttributes[tokenId].transferable is true.

However, safeBatchTransferFrom is not override in GameItems to add this check, so that malicious user can use this function to transfer GameItems even if allGameItemAttributes[tokenId].transferable is false.

Proof of Concept

malicious user can use safeBatchTransferFrom to transfer GameItems with transferable == false, see below code:

function testSafeTransferFrom() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        _gameItemsContract.adjustTransferability(0, false);

        // safeTransferFrom can't transfer if allGameItemAttributes[tokenId].transferable == false
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        uint256[] memory ids = new uint256[](1);
        ids[0] = 0;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;
        bytes memory data = new bytes(1);
        data[0] = "";
        // safeBatchTransferFrom can still do the transfer
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, data);
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Tools Used

Manual Review and Foundry

Recommended Mitigation Steps

Override safeBatchTransferFrom to add allGameItemAttributes[tokenId].transferable check

Assessed type

Token-Transfer