Protocol Review

The mint() function in FootiumGeneralMinting accepts a value parameter that represents the payment amount and an amountparameter representing the number of NFTs to mint. However, the payment validation logic does not multiply value by amount, allowing users to mint multiple NFTs while paying only for a single token.

User ends up severely underpaying since users pay a flat value regardless of how many NFTs they mint. The technical documentation in FootiumGeneralMinting.sol states:

// Setup: Anyone can mint up to 5 NFTs for 0.05 ETH each
// In merkle tree: [mintId, false, 0, 5, address(0), 0.05 ether]

However, the implementation contradicts this by treating value as a total payment rather than a per-NFT price.

Multiply the configured price by amount

“Non-holder” mints (excludeParentNftOwners == true, parentNftId == 0) only check _parentNft.balanceOf(receiver). A holder can still call mint and forward NFTs to a fresh wallet, bypassing the intent to prevent existing holders from minting.

If the goal is to block holders from participating at all, validate _parentNft.balanceOf(msg.sender) == 0 (or force receiver == msg.sender) when exclusion mode is used. Otherwise clarify in docs that the current check only prevents delivery to holder wallets, not holder participation.

Multi-minting a fixed tokenId reuses the same identifier inside the mint loop. Unless the underlying IFootiumMintable ignores the parameter (auto-increment mode), any call with amount > 1 and an explicit tokenId will revert on the second iteration, halting the entire mint process.

Ensure to derive unique IDs per iteration or require that amount == 1 when tokenId != 0.

During initialize the contract grants DEFAULT_ADMIN_ROLE to msg.sender, but transferOwnership does not reassign or revoke that admin role. Once the owner hands the contract to a multisig or new team, the previous owner still has full admin authority (can grant or revoke MINTER_ROLE, change other admins, etc.), while the new owner cannot manage minters because DEFAULT_ADMIN_ROLE never moved.

Update ownership transfers (or add a setter) so AccessControl admins mirror the current owner, and revoke the old admin when ownership changes.

None of the upgradeable contracts disable their initializers on the implementation contract. An attacker can call initialize on the implementation directly and become the implementation owner/admin. While this does not affect proxies directly, it enables phishing and operator confusion, and can be dangerous if the implementation ever receives ETH or is otherwise interacted with by mistake.

Add a constructor that calls _disableInitializers() in each upgradeable contract to lock the implementation.

The mint() function in FootiumGeneralMinting does not validate that amount > 0 before processing payment. A user can successfully call the function with amount = 0 while providing a non-zero value, resulting in payment being transferred to the payment receiver without any NFTs being minted to the user. The payment logic executes regardless of the amount, while the minting loop only executes if amount > 0. This creates a scenario where users can lose funds without receiving any tokens in return.

Add validation to require non-zero amount.

The FootiumPrizeDistributor and FootiumGeneralMinting contracts assume that ERC20 token transfers move the exact amount specified. This assumption breaks for tokens with non-standard transfer mechanics.

Document that only standard ERC20 tokens are supported

The merkle leaf construction in FootiumGeneralMinting, FootiumAcademy, and FootiumPrizeDistributor contracts does not include any chain-specific identifier such as block.chainid or a deployment salt. The leaf hashes only contain mint parameters like addresses, amounts, and IDs. If the protocol deploys the same merkle root across multiple chains for operational convenience, users can replay their valid merkle proofs on every chain where the contract is deployed.

Include a chain id for the proof if the plan is to go cross-chain.

FootiumGeneralMinting is an upgradeable contract (PausableUpgradeable, ReentrancyGuardUpgradeable, OwnableUpgradeable) but does not reserve storage slots for future variables. Without a _gap array, any future version that adds state variables will shift inherited storage layout and can corrupt parent storage.

Consider appending a gap to storage.

Per-user limits are keyed to receiver, not to the caller: _checkMintLimit updates _mintCounts[mintId][receiver] and the Merkle leaf omits receiver whenever isAddressReserved is false. A single account can therefore reuse the same proof indefinitely simply by changing the receiver argument to fresh addresses they control, defeating “per user” or “per proof” caps while still paying once per call. An arbitrary user can also intentionally mint to a victim’s address, consuming that victim’s entire mint allowance (although there may be little incentive to do this as the user would pay for the victim’s mint).

Consider if this is expected behavior for the protocol. If not, either bind the receiver into the proof even for public mints or track limits by msg.sender.

Function _handlePayment() assumes all ERC20 tokens return a boolean value from transferFrom(). However, some widely-used tokens like USDT (on Ethereum mainnet) do not return any value from their transfer() and transferFrom() functions. When Solidity attempts to decode an empty return value as a bool, the transaction will revert with an ABI decoding error.

Consider using SafeERC20 or set the _paymentToken carefully.

The contract currently supports receiving both native ETH and ERC-20 tokens. However, only native ETH can be recovered via the existing withdraw function. If ERC-20 tokens are accidentally transferred to the contract (for example, by users sending tokens directly), those tokens remain permanently stuck since there is no recovery mechanism for them.

Add an emergency withdrawal function for arbitrary ERC-20 tokens, restricted to owner role.

The FootiumPrizeDistributor contract allows the owner to update the merkle root at any time via setERC20MerkleRoot() and setETHMerkleRoot(). The claim functions calculate the claimable amount as: uint256 value = _amount - totalERC20Claimed[_token][_to];

If the owner updates the merkle root with a cumulative _amount that is lower than what the user has already claimed (totalERC20Claimed[_token][_to]), the subtraction will underflow and revert, blocking user claims.

Add validation to ensure merkle root updates don't reduce cumulative amounts below already-claimed totals.

The FootiumAcademy contract tracks minted players using a mapping keyed only by playerId: mapping(string => bool) private _mintedPlayers;

The contract checks require(_mintedPlayers[playerId] == false, "Player already minted"); so once the id is minted, it cannot be minted again.

However, the merkle proof verification includes both clubId and playerId: bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(clubId, playerId, msg.value))));

The off-chain merkle tree can legitimately contain multiple entries with the same playerId but different clubId values. However, once any club mints a player with a given playerId, no other club can mint a player with that same ID, even if they have a valid merkle proof.

Change the _mintedPlayers mapping to be keyed by both clubId and playerId.

The FootiumAcademy contract uses string type for player IDs without any normalization or case-insensitive comparison. The _mintedPlayers mapping stores player IDs exactly as provided, meaning "Player-123", "player-123", and "PLAYER-123" are treated as three completely different players. The merkle proof verification encodes the player ID string as-is, so case variations produce different merkle leaves and are considered valid distinct entries. If the off-chain merkle tree generation system doesn't enforce consistent casing, the same logical player can be minted multiple times with different case variations.

Ensure the off-chain system normalizes player ID's and does not multiple entries for the same player.

The claimETHPrize function transfers ETH using a low-level call without providing an alternative claim mechanism. The function enforces that only the prize recipient can claim (_to != msg.sender check), and if the recipient is a contract address without a receive() or payable fallback() function, the ETH transfer will always fail, locking the funds for the recipient.

Clearly document this behavior.

The NatSpec for mint states parentNftId … (0 or type(uint256).max to skip ownership check). However, the implementation only treats parentNftId == 0 as the skip condition. If the backend follows the documented type(uint256).max sentinel, calls will reach the ownerOf(parentNftId) branch and revert, permanently disabling those mint configurations.

Update the NatSpec or adjust the ownership check code in mint to be synchronized.

In the mint function, the excludeParentNftOwners input variable does not have a NatSpec, unlike the other variables.

Add a NatSpec @param excludeParentNftOwners entry that clearly explains the two behaviors (require ownership vs. exclude owners).

The initialize() function in FootiumGeneralMinting does not validate that the provided addresses are non-zero before assigning them to critical contract state variables.

Add zero-address validation for critical parameters in the initialize() function

The _checkParentNftOwnership() function only validates two specific combinations of parentNftId and excludeParentNftOwners parameters, leaving other combinations unchecked:

(1) parentNftId != 0 AND excludeParentNftOwners == true

(2) parentNftId == 0 AND excludeParentNftOwners == false

The intent of these parameter combinations are ambiguous. For example, if excludeParentNftOwners is true, the contract can enforce that the parent NFT balance of the receiver is 0, but that would be contradictory with the defined parentNftId having to be owned by a user.

Clearly document these no-op combinations.

The FootiumGeneralMinting contract cannot enforce transaction-level pacing or batch-size restrictions as the amount parameter is not included in the merkle proof. A user can mint their entire allocation in a single transaction (amount = limit) or split it across multiple calls.

Consider if this is okay for the purposes for the general minting contract and clearly document this behavior.

Parameters parentNft and paymentToken are not checked against the zero address unlike mintableNft and paymentReceiverAddress.

Consider adding zero address checks for those variables as well.

The recoverERC20 function emits an ERC20Recovered event, but the native ETH withdraw path does not emit any event.

Consider emitting a dedicated event for native ETH withdrawals to ensure parity with ERC20 recovery and improving observability for all recovery actions.

The NatSpec on mint: excludeParentNftOwners currently describes only one way to bypass the parent-NFT ownership check, but _checkParentNftOwnership actually supports multiple skip conditions. This discrepancy can mislead integrators into assuming ownership is enforced in cases where it is not.

Likewise, the value parameter is documented only as “The payment amount,” even though the total cost is computed as value * amount, which may cause confusion.

Simplify the NatSpec so it does not state the specific bypass combinations. Instead, keep the high-level description in mint general, and rely on the detailed four-case commentary already present inside _checkParentNftOwnership to define the exact behavior.

Also clarify in the NatSpec that value represents the per-NFT price, not the total payment.

It is possible for the same receiver to have multiple leaves and for the same tokenId but with different values. Consequently, a user can claim a specific NFT but for a cheaper price than encoded in another leaf. Furthermore, with multiple leaves and a specific tokenId, only the first user to mint will be able to receive that specific token.

Clearly document this behavior and ensure the Merkle tree is appropriately constructed to spec.

In global mint-count mode, uniqueness is only enforced if the Merkle leaf uses limit = 1. If limit = 0, the mint is unlimited and untracked; if limit > 1, the same global mintId can be minted multiple times.

Enforce limit = 1 for global mints (backend), and add contract-level protection to reject limit = 0 or limit > 1 when useGlobalMintCounts is enabled.

useGlobalMintCounts is intended to be a deployment-time mode flag, but it is currently stored as a normal storage variable.

Make useGlobalMintCounts immutable.

setPaymentReceiverAddress does not reject address(0), even though the constructor rejects a zero payment receiver. If the owner accidentally sets the payment receiver to zero, future mints will forward payments to address(0) .

Add the same zero-address validation to setPaymentReceiverAddress.