`safeTransfer()` with `data` not overridden

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) is not overridden to check _ableToTransfer() in FighterFarm transfers. Therefore it is possible to hold more than MAX_FIGHTERS_ALLOWED fighters and transfer staked fighters.

This may enable the user to initiate more than 10 battles per day for a fighter, and may DoS updateBattleRecord().

Proof of Concept

FighterFarm inherits from ERC721 which contains three transfer functions. Two of these are overridden to include a require(_ableToTransfer(tokenId, to));

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

prevents holding more than MAX_FIGHTERS_ALLOWED fighters and prevents transfers of staked fighters. But the public safeTransferFrom() which includes the data parameter is not overridden and therefore void of these new restrictions.

It is therefore possible to transfer fighters such that the recipient holds more than MAX_FIGHTERS_ALLOWED fighters, and to transfer staked fighters.

A consequence of this is that the new owner has a fresh voltage and battles can therefore be initiated without limit.

Since points are added to or deducted from accumulatedPointsPerFighter and accumulatedPointsPerAddress in parallel at every win or loss, transferring the fighter to a new address, for which then accumulatedPointsPerAddress is 0 (or just different) _addResultPoints() will attempt to remove more points then possible, since the points are calculated based on the tokenId rather than the fighterOwner. This will then cause a DoS of updateBattleRecord() due to underflow.

Recommended Mitigation Steps

Override _transfer(), _beforeTokenTransfer() or _afterTokenTransfer() instead.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #739

[c4-judge]

HickupHH3 marked the issue as satisfactory