Munchables - swizz'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 new shorter duration times for their locked funds and emit false events.

Proof of Concept

The setLockDuration function has the following if-statement to make sure that no user can set a new lock duration that is before the current unlockTime:

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

Then the function updates the new unlockTime immediately after like so:

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

The problem here is that we are using uint32(block.timestamp), instead of the previous lock time that was set, to check if a user is setting shorter durations. This will lead to shorter durations being set.

Let's imagine a user has already locked funds for a certain duration and wants to set a new duration. For example let's say the user currently has:

1. A previous last lock time at 500
2. An initial duration that was set at 1500
3. A current unlockTime at 2000 (500 + 1500 = 2000)

Now let's say the current block.timestamp is at 1900, and the user calls this function again, but with _duration being set to 150, lowered from 1500. Plugging in these numbers in the if-statement will not make it revert:

if(1900 + 150 < 2000) { revert LockDurationReducedError(); }

Now let's go ahead and update the unlockTime again by plugging in the new numbers and see what we get. The new unlock time is now:

lockedTokens[msg.sender][tokenContract].unlockTime = 500 + 150 = 650 650 < 2000

The event in this function will now emit a new duration of 150, and the new unlockTime is now reduced from 2000 to 650.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider changing the uint32(block.timestamp) to lockedTokens[msg.sender][tokenContract].lastLockTime correctly check the user isn't setting shorter duration periods:

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

Assessed type

Timing

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 role owner can possibly cast a disapprove vote and approve vote in the same USD proposal.

Proof of Concept

In the approveUSDPrice() function, a price feed role owner can cast an approve vote on a USD price proposal even if they already set a disapprove previously because there is no check to see if the msg.sender had previously disapproved the proposal. This will potentially cause both usdUpdateProposal.approvalsCount++ and usdUpdateProposal.disapprovalsCount++ to be incremented by the same user.

It's implied that a price feed role owner can only vote once and not have their vote changed because if we take a look at the disapproveUSDPrice() function, it checks to see if the msg.sender previously approved or disapproved. If either option was already made, then the function reverts. See code below inside disapproveUSDPrice():

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

The same must be done inside approveUSDPrice() function so that a user who previously disapproved a proposal can not decide to approve the same proposal

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a check inside approveUSDPrice() to make sure a user did not already disapprove before. Please add to the beginning of the function:

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

Assessed type

Other