Munchables - itsabinashb's results

Lines of code

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

Vulnerability details

Impact

The way lock duration is set by calling LockManager::setLockDuration() it is possible to set a lock duration which is less than minimum lock duration.

Proof of Concept

Suppose minimum lock duration is 60 seconds. A lock was created at timestamp X which is the lastLockTime of the lock. So unlockTime will be (X + 60 seconds).

  lastLockTime(X)                                                unlockTime(X+60)
      |-------------------------------------------------------------|
       <------------- duration = minDuration = 60 secs ------------->

Now, at the time of (x+40 secs) player called the setLockDuration() with 30 secs as argument. As (X + 40 + 30) = (x + 70) is greater than the unlockTime i.e (X+60) so the call will not revert by LockDurationReducedError. Finally unlockTime = lastLockTime + duration = X + 30. As of the new unlockTime the lock is already unlocked. So we can see that actually (X + 30) is far less than minimum duration, still the player managed to unlock 20 seconds before [because he called the setLockDuration() at (X + 40). The problem is in the process of checking the condition for whether the duration is reduced or not i.e LockDurationReducedError. The if condition looks like:

                if (
                    uint32(block.timestamp) + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                ) {
                    revert LockDurationReducedError();
                }

You can see that uint32(_duration) is added with block.timestamp but it should not, it should be added with lastLockTime. The new duration looks like:

                                           X + 40 secs
  lastLockTime(X)                      setLockDuration()         unlockTime(X+60)
      |--------------------------------------|-----------------------|
       <------------- duration = minDuration = 60 secs ------------->

      X                            X+30
      |-----------------------------|
      <----- new duration = 30 ----->

Tools Used

Manual review

Recommended Mitigation Steps

Change the if block from present to this:

                if (
                    lockedTokens[msg.sender][tokenContract].lastLockTime + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                ) {
                    revert LockDurationReducedError();
                }

Assessed type

Math