Munchables - tedox's results

Lines of code

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

Vulnerability details

Impact

When a user calls setLockDuration the unlockTime of his tokens can be increased more than they intended.

Proof of Concept

When a user calls setLockDuration the unlockTime for his tokens is calculated based on their lastLockTime which is set only when the function _lock() is called.

Lets say Bob wants to call setLockDuration in order to increase his lockDuration by 1 day. Alice sees this transaction in the pool and calls lockOnBehalf with parameters _quantity = 0, _onBehalfOf = {Bob's Address}. After Alice's transaction goes through the new lastLockTime of Bob's tokens will be block.timestamp instead of some time in the past.

Now when Bob's transaction starts executing. lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration); unlockTime will be bigger than it was supposed to be when Bob first called the function because lastLockTime will be block.timestamp instead of the time Bob locked his tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Calculate unlockTime by passing only the increase in the lock as a variable in setLockDuration instead of the total time. Avoid using lastLockTime and use the previous unlockTime to calculate the new unlockTime

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Users can shorten their lock time and have immediate access to their funds. Also they can bypass the minimumLockTime set by the contract.

Proof of Concept

The function setLockDuration() is supposed to allow users to only extend their current lock time by passing the new duration. The contract treats the variable _duration in 2 different ways. The first way is as the total duration of the lock since its beginning and as the duration to add from block.timestamp. Because of this differenece users are able to shorten their lock time.

Example: In the beginning Alice has a lock that has the following properties: unlockTime = 10, lastLockTime = 0 and the current block.timestamp = 6. She wants to change her lock time so she calls setLockDuration(5) with the value _duration = 5. She passes the check because 5+6>=10

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

That allows her to change her unlockTime which is then calculated as lastLockTime + _duration which is equal to 0 + 5 = 5. As a result her new unlockTime is actually in the past and the funds are no longer locked.

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

This can also be done n-1 times to bypass a lock that is n units in the future.

Tools Used

Manual review

Recommended Mitigation Steps

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

Assessed type

Math