Munchables - ke1caM's results

Lines of code

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

Vulnerability details

Vulnerability details

User shouldn't be able to reduce lock duration // check they are not setting lock time before current unlocktime.

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

        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);
    }

However it is possible that user reduces his lock duration, using setLockDuration function, due to wrong calulation of new unlock time. block.timestamp + _duration should be greater or equal to unlockTime.

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

New unlockTime is calculated based on lastLockTime not block.timestamp.

Proof of Concept

1. Let's assume that user deposited at block.timestamp = 100 (lastLockTime = 100) and his unlockTime = 150.
2. At block.timestamp = 125 user calls setLockDuration with 25 as an duration argument.
3. 125 + 25 = 150 which is equal to unlockTime (150).
4. New unlockTime is set to => lastLockTime (100) + duration (25) = 125, which is lower than 150. User reduced his lock duration and can unlock his tokens.

Impact

User can bypass core protocol functionality. He can reduce lock duration and withdraw his tokens earlier than he should.

Recommended Mitigation Steps

Fix calculation of new unlockTime in setLockDuration function.

You can do it by replacing lastLockTime with block.timestamp.

Assessed type

Other