Arbitrary Reduction of Lock Time in setLockDuration Function

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Users can exploit this vulnerability to reduce the lock duration of their tokens, allowing them to unlock tokens earlier than intended. This undermines the lock mechanism and allows users to receive rewards without adhering to the proper lock time, potentially leading to economic imbalances and unfair advantages.

Proof of Concept

Assume the following variables:

    currentTimestamp = 100
    (attackCall) _duration = 30
    lastLockTime = 80 (some point in the past where user locked tokens)
    pastDuration = 50 (original duration of the locked token at 80 of timestamp,lastLockTime)
    currentUnlockTime = lastLockTime + pastDuration = 80 + 50 = 130

If the current unlockTime is, for example, 130, this check will pass:

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

in fact this if statement will be: 130(100+30) < 130 => false = skip

and lastly will be reduced the unlockTime at this line:

lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration); // 80 +30 = 110 @audit reduced!

reducing in this way the unlockTime!

will leave the function commented to get a better idea:

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) < // 100 + 30 @audit - MEDIUM needs to check lastLockTime + duration lower than current unlockTime
                    lockedTokens[msg.sender][tokenContract].unlockTime // 130
                ) {
                    revert LockDurationReducedError();
                }

                uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
                    .lastLockTime;
                lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration); // 80 +30 = 110 @audit reduced!
            } // users can reduce duration of timelock
        }
        emit LockDuration(msg.sender, _duration);
    }

Tools Used

manual review

Recommended Mitigation Steps

2 options: 1) change this line:

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

to this:

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

2) change this if statement:

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

to this

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

Assessed type

Context

[c4-judge]

alex-ppg marked the issue as duplicate of #89

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)