Munchables - Kaysoft's results

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L275-L295 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L311-L398

Vulnerability details

Impact

A malicious user can use the lockOnBehalf function to extend the _lockDuration of other users so that they can not unlock and withdraw their deposits.

Proof of Concept

The main issue is that lockOnBehalf function allows a user to pass any address as _onBehalfOf parameter. The second issue is that _quantity can be zero.

With this two issues, it will only cost the attacker, the transaction fee to extend any user's lock duration by frontrunning their unlock transaction. And the transaction fee to achieve that on Blast network will not cost up to $1.

Below is the lockOnBehalf function with calls the _lock(...) internal function allowing the user to pass _onBehalfOf address parameter.

lockOnBehalf(...)

File: LockManager.sol

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)) {
289:            lockRecipient = _onBehalfOf;
        }

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

Online 289 above, _onBehalfOf is assigned to the lockRecipient then the lockRecipient is passed to the internal _lock. Since any user can execute transaction to updated the state of lockRecipient, An attacker can take advantage of this by calling the lockOnBehalf function with the victim's address as the _onBehalfOf parameter. The attacker can also pass zero as the _quantity parameter which will make it even cheaper to exploit since _quantity is not validated against zero values.

Exploit Scenario

1. Bob locks 100 ETH on LockManager.sol
2. When lock duration is reached Bob sends a transaction to unlock his 100ETH
3. Alice sees Bob's transaction in the mempool and frontruns it with a call to lockOnBehalf function passing Bob's address as _onBehalfOf parameter and zero as _quantity
4. Alice's transaction is successful and extend's Bob's lock duration
5. Bob's transaction reverts because Bob's lockedToken.unlockTime has been extended to a new timestamp in the future by Alice's transaction in #3 & #4
6. Alice can continue doing this to Bob causing Bob's 100ETH to be stuck in the contract for as long as possible.
7. It only costs Alice less than a dollar to execute this attack on any user. Alice can extend any user's lock duration with the lockOnBehalf function at any time without even waiting for the user to want to unlock their asset.

Here is the _lock internal function: https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L311C5-L398C6

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the lockOnBehalf function since the _onBehalfOf address parameter of the function can be used to extend lockedToken.unlockTime of users.

Assessed type

Other