Munchables - stakog's results

Lines of code

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

Vulnerability details

Description

In the Munchables game, registered users can lock their tokens for a selected period of time in order to receive Schnibbles (food to grow the munchables) and also earn new Munchable NFTs.

While Schnibbles depend only on the amount locked, to earn new NFTs users must lock their tokens for a minimum duration during the Lockdrop period, as can be seen here (note that a maximum duration is enforced as well but it’s not relevant to this report).

Users lock their tokens by calling LockManager::lock or LockManager::lockOnBehalf , which will call LockManager::_lock to actually lock the tokens and enforce a minimum and maximum locking duration if the current time is within a Lockdrop period. If this is the first time a registered user calls the lock functions, the code will set the duration to the minimum value required by the Lockdrop period, as can be seen here.

If users want to specify the duration of their lock, then they can call the function LockManager::setLockDuration. According to the README The most important thing is that funds cannot get locked forever, people cannot take other people's funds, and that people cannot reduce lockup times that are previously set. So we can understand from this description regarding lockup times, that when setting a new duration for locks, this new duration cannot be such that it unlocks the tokens earlier then the current unlock time. Any new duration set by a user can only extend the lock duration (or at least not change it). The error seen here also reinforces this.

The issue here arises from the fact that this important functionality is broken and users can reduce their lockup times by as much as half. This essentially bypasses the Lockdrop’s minimum duration and users can unlock their tokens at time that is half of the minimum duration the Lockdrop tries to enforce.

This happens because there is a mismatch in LockManager::setLockDuration between the check in the if statement and the actual storage update of the new unlock time. While the if statement checks for block.timestamp + duration, the storage updates according to lastLockTime + duration. Unless the user calls LockManager::setLockDuration in the same block that they call one of the lock functions, block.timestamp will always be greater then lastLockTime. This allows a user to manipulate and therefore reduce the unlock time.

Let’s see an example: A user locks tokens for the first time and so their locking duration is set to the minimum which the sponsor stated is going to be 30 days. The user waits for 15 days and then calls LockManager::setLockDuration with a duration of 15 days. Since 15 days have passed and the duration the user passed is 15 days more, a total of 30 days is calculated and the if statement will not revert. When storage will be updated it will take the original lock time 15 days ago and will add the duration of 15 days to it, which will effectively equal to the current time (because remember we waited 15 days before executing the attack). The user now can call LockManager::unlock to receive their tokens back before the minimum duration has passed.

The following is a mathematical treatment to prove that an attacker can reduce the lockup time by a maximum of half

1. Consider the if statement condition:

uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime

Using simple notation, we can rewrite the condition as Td < Tu - Tc where Td is the new duration we pass, Tu is the original unlock time and Tc is the current time represented by block.timestamp.

2. Consider the storage update equation:

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

Using simple notation, we can rewrite the equation as Tnu = Tl + Td where Td is the new duration we pass, Tnu is the new calculated unlock time and Tl is the original locking time.

3. We want the new unlock time Tnu to be equal to the current time Tc so that we will be able to immediately unlock the tokens. This means we want Tnu = Tc.
4. We can represent the current time Tc as the locking time Tl + a fraction of the original duration that was set which we will call xTod where x represents a number between 0 and 1 and Tod is the original duration. So we have Tc = Tl + xTod
5. If we combine (2) and (4) into (3), we get → Tnu = Tc ⇒ Tl + Td = Tl + xTod ⇒ x = Td/Tod
6. We want to pass in (1) the minimum duration Td that will satisfy the condition so we write Td = Tu - Tc
7. We’ll use (4) on (6) and get → Td = Tu - Tc ⇒ Td = Tu - (Tl + xTod) ⇒ Td = (Tu - Tl) - xTod, and since Tu - Tl is the original unlock time minus the original locking time which is just the same as the original duration we can write Td = Tod - xTod ⇒ Td/Tod = 1-x.
8. Apply (7) on (5) and we get x = Td/Tod ⇒ x = 1 - x ⇒ x = 0.5

So we proved that the maximum reduction of the lockup time is 0.5 of the original duration that was set. If we set the original duration to 10 days, an attacker will be able to unlock their tokens after 5 days only.

Impact

Users can reduce their lockup times by as much as half. Users bypass the Lockdrop enforcement of a minimum duration and unlock their tokens in half the time of the minimum duration. As stated by the README, this is one of the most important functionalities and so should be considered a breaking of core functionality.

Proof of Concept

First add the following piece of code to MunchablesTest::logLockedTokens in MunchablesTest.sol so we will get logs of the unlocktime variable as well:

console.log("Unlock Time:", lockedTokens[i].lockedToken.unlockTime);

Next add the following test function to MrTester contract in SpeedRun.t.sol:

        function test_ReduceUnlockTime() public {
        uint256 lockAmount = 100e18;

        console.log("Beginning run()");
        deployContracts();

        // register me
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        logSnuggery("Initial snuggery");

        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);
        lm.setLockDuration(30 days);
        logLockedTokens("After initial lock");

        vm.warp(15 days + 1); // move time by half of the 30 days duration
        console.log(block.timestamp);

        lm.setLockDuration(15 days);
        logLockedTokens("Unlock time reduced"); 
        console.log("Unlock time has been reduced by half of the original duration and is equal to the current time");
    } 

Tools Used

Manual review

Recommended Mitigation Steps

Change the if condition in LockManager::setLockDuration to:

	-           uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime
	+           lockedTokens[msg.sender][tokenContract].lastLockTime + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime

This way the duration will be with respect to the original locking time.

Assessed type

Math