Munchables - carrotsmuggler's results

Lines of code

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

Vulnerability details

Impact

Users can set their own lockDuration to any value by calling the setLockDuration function. The only requirement is that they must not have any locks which are about to expire outside the new window in the future.

for (uint256 i; i < configuredTokensLength; i++) {
    //...
    if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
        //...
        if (uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime) {
            revert LockDurationReducedError(); //@audit rejects setting if it effectively lowers any lock duration
        }
    }
}

So if a lock is about to expire in 10 days, a user cannot set the new _duration to less than 10 days. This is done to make sure that the locks are not shortened in any way.

The issue is that other users can open locks on behalf of this user via the lockOnBehalf function. So they can open new locks, which will prevent the victim from shortening their duration.

Since any user can stop other users from reducing their lock duration, this can be considered a high severity issue.

Proof of Concept

Say Alice is a player, and has her current lock duration set to 10 days. She wants to reduce it to 5 days.

Alice has no locks active. So she should be able to call the setLockDuration function with a new _duration of 5 days.

However, Bob calls the lockOnBehalf function with Alice's address and locks some tokens for 10 days. Now, when Alice tries to call the setLockDuration function with a new _duration of 5 days, the transaction will revert. This is because the lockedTokens[msg.sender][tokenContract].unlockTime is set to be 10 days away, but block.timestamp + _duration is only 5 days away.

So Alice is unable to reduce her lock duration.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider disabling the lockOnBehalf function. Or, add a check so that only a set of addresses approved by the user can use lockOnBehalf for the target user.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Users can unlock after their lock period ends. The unlock time is denoted by the unlockTime field of the struct. This check is done in the unlock function.

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

However, when a user creates a lock over an existing lock, the time gets reset. The previous unlock time is overwritten with the new unlock time. This is done in the lock function.

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

So if a fresh lock is opened, the unlockTime is refreshed and pushed back by the duration amount. The issue is that any user can trigger this via the lockOnBehalf function. This function allows users to lock tokens on behalf of other users. This will also reset the lock time.

So if a user waits untl the unlock time to unlock their stake, another user can come in and reset the lock time by depositing a single wei through the lockOnBehalf function. The original user will not be able to unlock their stake until the new lock time is reached.

Since this prevents unlocks, this can be considered a high severity issue.

Proof of Concept

Say Alice has a lock expiring today. Her lockDuration is set to 10 days.

Bob calls the lockOnBehalf function with Alice's address and locks some tokens. Alice's unlockTime is now pushed back by 10 days.

Alice is unable to unlock her tokens. This can be done indefinitely to prevent Alice from unlocking her tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider removing the lockOnBehalf function. Or, add a check so that only a set of addresses approved by the user can use lockOnBehalf for the target user.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

When a user creates a lock, if their lockDuration was set to 0, their lock duration will get set to the minimum value.

if (_lockDuration == 0) {
    _lockDuration = lockdrop.minLockDuration;
}
playerSettings[_lockRecipient].lockDuration = _lockDuration;

This is present in the _lock function. the issue is that this can be trigerred by other users, via the lockOnBehalf function.

If a user has their lock duration set to 0, either because its a fresh account, or if they had deliberately set it as such, their lockduration can be reset to the minimum value by any other user.

Since a different user is able to manipulate the settings of a user's account, this is a medium severity issue.

Proof of Concept

Say Alice hsa her lockDuration set to 0. Bob can call the lockOnBehalf function with Alice's address, which will reset her lock duration to the minimum value.

Tools Used

Manual code review

Recommended Mitigation Steps

Consider disabling the lockOnBehalf function. Or, add a check so that only a set of addresses approved by the user can use lockOnBehalf for the target user.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The function setLockDuration can be called to change the duration of locks that the user wishes to have applied to their locks by default. This function should make sure that users cannot reduce the lock times of their locks retroactively, so puts in a safeguard for the same.

Hypothetically, if users were able to reduce their lock times, that would be problematic. The system gives out rewards/incentives based on the duration of the locks, so users can game the system by creating long locks, collecting the benefits, and then reducing the lock time. This should not be allowed by the system.

In this contract, such a safeguard is implemented, but it is not implemented correctly.

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

The lock time is set to the lastLockTime + duration This can actually reduce the lock time.

Proof of Concept

Lets say a lock exists. It was created on Jan 1, and had a duration of 10 days. Thus, the unlock time is Jan 11.

On Jan 7, Alice calls setLockDuration with a duration of 5 days.

block.timestamp + duration = Jan 7 + 5 days = Jan 12. This is higher than the unlockTime, which is Jan 11. So no revert happens.

lastLockTime is set to the previous lock time, of Jan 1.

unlockTime is set to lastLockTime + duration = Jan 1 + 5 days = Jan 6.

Alice's lock, which was about to expire on Jan 11, is now immediately available!

Tools Used

Manual analysis

Recommended Mitigation Steps

The new lock time should be block.timestamp, not the old lock time. This will basically create a fresh lock from today. In the above POC, if the lastLockTime was set to block.timestamp, the new unlock time will be Jan 7 + 5 days = Jan 12 (since block.timestamp is Jan 7). This will ensure that the unlock time never decreases.

Change the code to:

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Oracles are able to put up a proposal for a price update, and then the other oracles can vote on it. This is done via the approveUSDPrice and the disapproveUSDPrice functions.

After an oracle approves a proposal, they cannot disapprove it. This is ensured by the following check:

function approveUSDPrice(uint256 _price) {
    usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
}

function disapproveUSDPrice(uint256 _price) {
    if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId) {
        revert ProposalAlreadyApprovedError();
    }
}

So if a userhas already voted for a price, they cannot vote against it.

However, the opposite check is not present. If a user votes against a proposal, their name is marked in the usdUpdateProposal.disapprovals mapping. However, this is not checked in the approveUSDPrice function. This means that a user can vote against a proposal, and then vote for it, and their vote will be counted both times.

Since an oracle gets two vote twice, this is considered manipulation of the voting system.

Proof of Concept

Due to the missing check, Alice, an oracle, can first vote against a proposal. then she can also vote for that proposal, since the usdUpdateProposal.disapprovals[Alice] value is not checked.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check in the approveUSDPrice function to ensure that a user cannot vote for a proposal if they have already voted against it.

Assessed type

Invalid Validation