Munchables - Sabit's results

Lines of code

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

Vulnerability details

Impact

A malicious user can call the lockOnBehalf function with a valid _onBehalfOf address and a _quantity of zero, effectively extending the lock duration of the _onBehalfOf address without actually locking any tokens.

Proof of Concept

The lockOnBehalf function allows a user to lock tokens on behalf of another address (_onBehalfOf). However, the function does not check if the _quantity parameter is greater than zero.

This allows a malicious user to pass a valid _onBehalfOf address and a _quantity of zero, effectively extending the lock duration of the _onBehalfOf address without actually locking any tokens.

Tools Used

Manual review

Recommended Mitigation Steps

In the lockOnBehalf function, add a check to ensure that the _quantity parameter is greater than zero before calling the _lock function. This will prevent the lock duration from being extended without actually locking any tokens.

if (_quantity == 0) revert ZeroQuantityError();

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

unlockTime can be shortened when lock duration is extended

Proof of Concept

The issue lies in the calculation of the new unlockTime for the locked tokens.

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

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

The code assumes that the lastLockTime stored in the lockedTokens mapping represents the time when the tokens were last locked. However, it does not consider the possibility that the user may have set a different lock duration in the past, which could have already extended the unlockTime.

In this case, simply adding the new _duration to the lastLockTime would result in an incorrect unlockTime.

Suppose a user, Alice, has locked some tokens for a duration of 30 days (2,592,000 seconds) on January 1st, 2023, at 12:00:00 UTC. Initially, the lockedTokens mapping would be:

lockedTokens[Alice][tokenContract] = { quantity: 1000, lastLockTime: 1672531200, // January 1st, 2023, 12:00:00 UTC unlockTime: 1675123200, // January 31st, 2023, 12:00:00 UTC (lastLockTime + 30 days) remainder: 0 }

Now, let's say on January 15th, 2023, at 12:00:00 UTC, Alice wants to extend the lock duration to 45 days (3,888,000 seconds). According to the current implementation, the code would calculate the new unlockTime as follows:

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

The new unlockTime would be set to 1676419200, which corresponds to February 14th, 2023, 12:00:00 UTC.

However, this calculation is incorrect because it does not consider the original unlockTime of January 31st, 2023, 12:00:00 UTC. The correct unlockTime should be calculated based on the current block.timestamp (January 15th, 2023, 12:00:00 UTC) plus the new lock duration of 45 days.

The correct unlockTime should be 1677654000, which corresponds to March 1st, 2023, 12:00:00 UTC, as expected.

Tools Used

Manual review

Recommended Mitigation Steps

The new unlockTime should be calculated based on the current block.timestamp and the new _duration.

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

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

It's possible for an authorized caller to call disapproveUSDPrice and then call approveUSDPrice. This means an authorized caller can have disapproval and approval counts at the same time.

This can even lead to a situation where usd price can't be updated. For instance, if the number of authorized callers is 5. One user approves and disapproves a usd price, the four other callers approve and disapprove equally (2 for, 2 against).

With the initial user having an "approve" and "disapprove", "approve" would be 3 likewise "disapprove".

Proof of Concept

An authorized caller can call disapproveUSDPrice funtion first. Then, call approveUSDPrice function after.

Unlike in disapproveUSDPrice function, there's nothing stopping an authorized caller, who has called disappproveUSDPrice function initially, from calling approveUSDPrice function.

In essence, an authorized caller can have his "vote" count for approve usd price and disapprove usd price.

Tools Used

Manual review

Recommended Mitigation Steps

In approveUSDPrice function, also check if msg.seder has already called disapproveUSDPrice function.

Assessed type

Access Control