Munchables - turvy_fuzz's results

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L383 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L290

Vulnerability details

Impact

Attacker can constantly keep increasing the unlocktime of any user without needing permission, prevent any user they wish from unlocking

Proof of Concept

The lockOnBehalf() function allows anyone to lock on behalf of any user:

function lockOnBehalf(
        address _tokenContract,
        uint256 _quantity,
        address _onBehalfOf
    )
        external
        payable
        notPaused
        onlyActiveToken(_tokenContract)
        onlyConfiguredToken(_tokenContract)
        nonReentrant
    {
        address tokenOwner = msg.sender;
        address lockRecipient = msg.sender;
        if (_onBehalfOf != address(0)) {
  @>        lockRecipient = _onBehalfOf;
        }

        _lock(_tokenContract, _quantity, tokenOwner, lockRecipient);
    }

note that there is no minimum amount required to call this function, meaning it can be calling with 1wei. Also note that there is no permission required to call the function for any user. The issue here is that on end of the _lock() function it updated the recipients unlockTime:

        lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

This allows attackers the ability to keep increasing the unlocktime of any user without needing permission, prevent any user they wish from unlocking.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow users to grant permission on who can lock onbehalf of them

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L257 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L266

Vulnerability details

Impact

People can reduce lockup times that were previously set. Breaking the core invariant of the protocol as mentioned in the doc:

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.

Proof of Concept

The setLockDuration() allows users to update their lockDuration however must not be able set the new lock time before current unlocktime, this check is done here:

// check they are not setting lock time before current unlocktime
                if (
@>                  uint32(block.timestamp) + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                ) {
                    revert LockDurationReducedError();
                }

Notice it uses block.timestamp in comparing the new time in the if() condition. However, upon making the actual update it uses lastLockTime:

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

This allows user to reduce their unlockTime after some time has passed.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider making this changes:

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

Assessed type

Other