Munchables - trachev's results

Lines of code

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

Vulnerability details

Impact

By utilizing the lockOnBehalf function any user can be prevented from ever unlocking their locked tokens. The attack can be continuously performed as the cost of execution will be extremely low.

Proof of Concept

The lockOnBehalf function allows any user to lock tokens on behalf of another user. The issue is that when lockOnBehalf is called, the recipient of the locked tokens has their lockedToken.unlockTime reset to block.timestamp + _lockDuration:

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

As the unlock function reverts if lockedToken.unlockTime > block.timestamp, the following attack opportunity opens:

1/ User locks tokens for 90 days. 2/ 90 days pass and the user calls unlock. 3/ An attacker front-runs the call to unlock, locking 1 wei of the locked token on behalf of the user. 4/ lockOnBehalf resets the lockedToken.unlockTime to 90 more days in the future and unlock fails to execute. 5/ The victim cannot unlock their tokens for 90 more days and is likely to be front-runned again in the future.

It's important to note that the attacker may not even need to supply the 1 wei of the locked token as calling lockOnBehalf with quantity equal to 0 will also execute successfully.

Tools Used

Manual review

Recommended Mitigation Steps

Users should be able to specify which addresses are allowed to lock on their behalf.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

By utilizing the lockOnBehalf function any user can be prevented from ever updating their lock duration. The attack can be continuously performed as the cost of execution will be extremely low.

Proof of Concept

The lockOnBehalf function allows any user to lock tokens on behalf of another user. The issue is that when lockOnBehalf is called, the recipient of the locked tokens has their lockedToken.unlockTime reset to block.timestamp + _lockDuration:

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

As the setLockDuration function reverts if:

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

An attacker can front-run the call to unlock, locking 1 wei of the locked token (or even 0) on behalf of the user. Thus increasing their unlockTime, causing the execution to revert. The victim will not be able to update their lock duration for a prolonged period of time and is likely to be front-runned again in the future.

Tools Used

Manual review

Recommended Mitigation Steps

Users should be able to specify which addresses are allowed to lock on their behalf.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

In the setLockDuration there is a check to make sure that the lock duration is not reduced while there are still tokens that are locked. The issue is that the validation is insufficient.

Proof of Concept

As we can see, this check makes sure that the new duration of the lock is not less than the previous one:

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

The problem is that the previous unlockTime is compared to the current time plus the new duration when it should be compared to the time of the last lock plus the new duration. This allows for the new unlockTime of the currently locked tokens to be less than the previous one. We understand from the validation above that this should not be permitted. Here is an example:

1/ The current lock duration of some locked tokens is 90 days 2/ We will assume, for simplicity, that the tokens were locked on block.timestamp equal to 0 days 3/ 40 days pass and the user decides to reduce the lock duration to 60 days 4/ The call does not revert in the check above as the current timestamp is at 40 days, which added to the new duration is more than the unlock time (100 days > 90 days) 5/ The unlock time is now set to 0 days (lastLockTime) plus 60 days, equal to 60 days 6/ Therefore, the lock duration was decreased successfully, even though from the validation shown above we can see that this should not be allowed.

Tools Used

Manual review

Recommended Mitigation Steps

There are two ways to solve this issue depending on the protocols intentions: a/ The check above should use: lastLockTime + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime OR b/ The new unlockTime should be set to block.timestamp + uint32(_duration)

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

A price feed can either approve or disapprove the USD price of a locked token. The issue is that currently, a price feed can vote for both of the options at the same time, rendering the price feed's vote pointless.

Proof of Concept

As we can see the disapproveUSDPrice function reverts if the price feed has already approved or disapproved the current price proposal:

if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
   revert ProposalAlreadyApprovedError();
if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
   revert ProposalAlreadyDisapprovedError();

The second check is missing in the approveUSDPrice function.

Tools Used

Manual review

Recommended Mitigation Steps

Make sure that the price feed has not already disapproved the price proposal in approveUSDPrice.

Assessed type

Other