Munchables - dhank's results

Lines of code

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

Vulnerability details

Impact

TokenOwner can increase the lock duration of the token Receiver without adding any funds on behalf of the receiver.

Proof of Concept

A player A can send 0 quantity on behalf of another player B. And as a result player B is locked with 0 amount for uint32(block.timestamp) + uint32(_lockDuration) time

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

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

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L311-L399 function _lock( .... ) 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.remainder = remainder;
    lockedToken.quantity += _quantity;
    lockedToken.lastLockTime = uint32(block.timestamp);
    lockedToken.unlockTime =
        uint32(block.timestamp) +
        uint32(_lockDuration);

    ....
}

Hence , one player can lock another player's fund for indefinite time if they wanted to.

Tools Used

Manual Review

Recommended Mitigation Steps

If the _quantity is 0 , revert

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

Lock up time that are previously set can be reduced and assigned to a past time ,

Proof of Concept

Function for extending LockDuration.

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

    function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
            revert MaximumLockDurationError();


        playerSettings[msg.sender].lockDuration = uint32(_duration);
        // update any existing lock
        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {
            address tokenContract = configuredTokenContracts[i];
            if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
                // check they are not setting lock time before current unlocktime
                if (
                    uint32(block.timestamp) + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                ) {
                    revert LockDurationReducedError();
                }


                uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
                    .lastLockTime;
                lockedTokens[msg.sender][tokenContract].unlockTime =
                    lastLockTime +
                    uint32(_duration);
            }
        }


        emit LockDuration(msg.sender, _duration);
    }

• User has locked their fund at timestamp X for y duration.So the new unlockTime became X + y and lastLocktime became X;

• User tried to extend their locktime using setLockDuration to a new duration z which is LESS THAN y.

• line #256 , function will check for block.timestamp + z < unlockTime , which can be false when the block has reached to that time near to the unlockTime ( block.timestamp + z > X + y ) . Hence it wont revert and continues the execution.

• line #265 ,new unlockTime gets the value X + z which is clearly less than X + y which is the initial unlockTIme .

unlockTime is reduced here and can possibly be reduced to a past time.

Tools Used

Manual review

Recommended Mitigation Steps

Instead of checking

   if (
           uint32(block.timestamp) + uint32(_duration) <
           lockedTokens[msg.sender][tokenContract].unlockTime
       ) {
           revert LockDurationReducedError();
       } 

function should check

    if (
            lockedTokens[msg.sender][tokenContract].lastLockTime + uint32(_duration) <
            lockedTokens[msg.sender][tokenContract].unlockTime
        ) {
            revert LockDurationReducedError();
        } 

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

A User can Approve the proposed Price even though he has disapproved before and no decrement is done for disapprovalCounts.

Proof of Concept

In the below code snippet the function is not checking whether the msg.sender has already disapproved.

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

    function approveUSDPrice(uint256 _price) external onlyOneOfRoles([ ....])
    {
        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++;

       ....

    }

As a result any user who is already disapproved can approve the Proposal thus making the voting power of other roles irrelevant

Tools Used

Manual Review

Recommended Mitigation Steps

If contract doesnt want to allow a user who has already disapproved to approve the proposal. Function should add this check

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

Else if , the contract allows a user who has already disapproved to approve the proposal. Function should the code to decrement the usdUpdateProposal.disapprovalsCount and set the usdUpdateProposal.disapprovals[msg.sender] to 0;

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

User can earn extra nfts than deserved.

Proof of Concept

The unlock function is not updating the lockedToken.remainder.

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

    function unlock(address _tokenContract,uint256 _quantity) external notPaused nonReentrant {
        
        ....

        lockedToken.quantity -= _quantity;


        // send token
        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);
        } else {
            IERC20 token = IERC20(_tokenContract);
            token.transfer(msg.sender, _quantity);
        }


        emit Unlocked(msg.sender, _tokenContract, _quantity);
    }

Suppose the _quantity to unlock here is lockedToken.quantity - 'X' where 0 <= 'X' < lockedToken.remainder.

At the end of the function,the _quantity is transfered to the user whcih includes lockedTocken.remainder.

Since the lockedToken.remainder is not updated , for the next nft calculation (when the user locks new _quantity) , _quantity + lockedToken.remainder is used for creating new NFTs even though some/all (depends on 'X') of the lockedToken.remainder was already transfered to the users account when they unlocked before.

When user call locks new quantity ,

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

        function _lock(address _tokenContract,uint256 _quantity,address _tokenOwner,address _lockRecipient) private {
        
        ....

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

        ....
      
    }

in the line#344 , lockedToken.remainder is added to the newly added funds to calculate the total quantity when some or all of the lockedToken.remainder was actually got transfered to players account.

Which means with the same amount in hand, user can make use of this vulnerability to create more Munchablenfts than deserved.

Tools Used

Manual review

Recommended Mitigation Steps

Fucntion should change the locked.remainder accordingly if the _quantity to unlock is greater than (lockedToken.quantity - locked.remainder)

Assessed type

Context