Munchables - cats's results

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L401 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L275 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L382-L384

Vulnerability details

Impact

When a user wants to unlock their tokens, the function makes sure the unlockTime has passed:

    if (lockedToken.unlockTime > uint32(block.timestamp))
            revert TokenStillLockedError();

The problem is that anyone can lock tokens on behalf of someone else, and when a new lock is made, the unlockTime is refreshed with a new duration:

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

Not only that, but there is no require(_quantity > 0) check either, so an attacker can virtually grief any vicitm they choose from ever unlocking their tokens and the only cost of attack is paying gas. It's a permanent loss of funds and it costs only gas to make the attack so I believe high is appropriate.

Proof of Concept

Simple issue

Tools Used

Manual review

Recommended Mitigation Steps

Even if a minimum amount of 1 wei was set, this would still cost nothing and can be pulled off. One idea is to only allow someone to lock on behalf of someone else, if they have previously approved them through a function.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When a user locks their tokens, the timestamp of their lock is logged as lastLockTime:

    lockedToken.lastLockTime = uint32(block.timestamp);

If a user wants to update their lock duration (as long as it's not a timestamp before the already set unlockTime), they can call setLockDuration() in order to update their unlock time:

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

So when a user wants to set a new lock duration, the unlockTime is actually updated to lastLockTime + _duration and not block.timestamp + _duration. As long as the new updated duration is > the already set unlocked time, then all is good and it will be updated. The issue comes from the fact that when someone locks tokens on behalf of someone else, their lastLockTime is updated again, so an attacker can grief their victim, let's follow a simple example to demonstrate:

1. Alice locks 10 tokens and her unlockTime is set to 30 days from now, her lastLockTime is set to block.timestamp (let's call it 0th day)
2. 25 days pass and Alice wants to update her lock time with an additional 5 days, so her new unlockTime = lastLockTime + _duration, so intention is to set to 35 days from the original 0th day instead of 30
3. Any attacker that wants to grief Alice can locks on her behalf, even with amount == 0 which updates the lastLockTime to block.timestamp
4. Alice's tx goes through and now her unlockTime = block.timestamp + _duration, which is now a new 35 days from now, making the total lock time she has to wait 25 passed days + 35 new ones due to attack, she is forced to wait her lock time for a total of 60 days instead of her intended 35 days

The attacker can front run her or just spam 0 amount locks repeatedly and the grief is achieved. This is both possible with native msg.value 0, and token amount 0.

Proof of Concept

Fairly simple, can provide PoC if requested

Tools Used

Manual review

Recommended Mitigation Steps

Even if a check for require(_quantity > 0) is implemented, this can still be achieved with 1 wei, but at least its a good starting point. Deeper refactoring of the code might be needed to come up with a foolproof solution.

Assessed type

Timing