Munchables - adam-idarrha's results

Lines of code

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

Vulnerability details

Summary

The protocol allows users to lock tokens to earn rewards and mint NFTs. However, a critical flaw in the lockOnBehalf function permits any user to indefinitely extend the lock duration of another user's funds, potentially leading to denial of service (DOS) as users cannot access or unlock their locked tokens. and also allowing to DOS the ability of user to change their lockDuration using setLockDuration

Vulnerability Detail

The lockOnBehalf function enables a user to lock tokens on behalf of another user, calling the internal _lock function to handle the locking mechanism:

• lockOnBehalf

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

Within _lock, the unlock time for the funds is reset based on the current block timestamp plus the designated lock duration, effectively allowing the resetting of the lock period:

• _lock

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;

This mechanism can be exploited by malicious actors to continually extend the lock duration by initiating a new lock just before the unlock time is reached:

• unlock

function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        LockedToken storage lockedToken = lockedTokens[msg.sender][
            _tokenContract
        ];
        if (lockedToken.quantity < _quantity)
            revert InsufficientLockAmountError();
=>        if (lockedToken.unlockTime > uint32(block.timestamp))
            revert TokenStillLockedError();

        // force harvest to make sure that they get the schnibbles that they are entitled to
        accountManager.forceHarvest(msg.sender);

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

The attacker can effectively prevent the intended unlock of funds by repeatedly using lockOnBehalf to extend the lock duration, causing a denial of service.

• _lock

if (_lockDuration == 0) {
            _lockDuration = lockdrop.minLockDuration;
        }

the attacker can extend the lock duration while locking only 1 wei.

he can also DOS the setLockDuration by locking before the user calls it , leading to a revert in this condition:

• setLockDuration

if ( uint32(block.timestamp) + uint32(_duration) <
lockedTokens[msg.sender][tokenContract].unlockTime ) 

Proof Of Concept

• assumptions

  • Alice has locked for 30 days , 1 ETH
  • Alice's lockDuration is 30 days

• Lock Funds

  1. after 30 days passed Alice tries to call unlock
  2. Bob frontruns her and calls lockOnBehalf(address(0), 1, Alice)
  3. this locks her 1 ETH for another 30 days
  4. Alice's transaction reverts and she needs to wait for another 30 days
  5. Bob can keep any funds she deposited lcoked indefinetly

• DOS of changing lockDuration

  1. Alice wants to change her lockDuration from 30 days to 15 days
  2. Alice calls setLockDuration(15 days)
  3. Bob frontruns here and calls lockOnBehalf(address(0), 1, Alice)
  4. her setLockDuration will revert because of this check if ( uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime )
  5. Bob can keep DOS her lockDuration changes indefinetly

Impact

DOS for users trying to unlock their funds , and permanent locking of users funds, and ability to change the lockDuration which impacts his schnibble reward amount.

Tool used

Manual Review

Recommendation

do not allow other users to stake on behalf of another user, remove the function lockOnBehalf

Assessed type

DoS

Lines of code

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

Vulnerability details

Summary

The protocol allows users to lock funds to mint NFTs, requiring a minimum lock duration. However, the setLockDuration function permits users to reduce their lock duration by half, circumventing the intended minimum lock period required for NFT minting.

Vulnerability Detail

The _lock function is responsible for users locking their tokens to mint NFTs, with validation against the minimum and maximum lock durations:

• _lock

if (
            lockdrop.start <= uint32(block.timestamp) &&
            lockdrop.end >= uint32(block.timestamp)
        ) {
=>            if (
                _lockDuration < lockdrop.minLockDuration ||
                _lockDuration >
                uint32(configStorage.getUint(StorageKey.MaxLockDuration))
            ) revert InvalidLockDurationError();

To mint an NFT, users must meet the minLockDuration requirement. The unlock time is initially set as follows in the lock function :

lockedToken.unlockTime =
    uint32(block.timestamp) +
    uint32(_lockDuration);

The setLockDuration function allows users to modify the duration of their locks, crucially without altering the initial unlock time for existing locks:

• setLockDuration

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

The vulnerability arises because the check (uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime) allows users to effectively halve their lock duration post-lock, bypassing the minimum lock period for benefits such as NFT minting,because the check checks block.timestamp while applying it on lastLockTime.

Proof Of Concept

• assumptions:

  • minLockDuration = 30Days
  • nftPrice in ETH is 1ETH

• attack Scenario

  1. Bob locks 1ETH for 30 days, qualifying for NFT minting.
  2. lastLockTime is set to the block.timestamp when he locked
  3. after 15 days, Bob calls setLockDuration(15 days)
  4. if ( uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime ) this check will pass because block.timestamp + 15 days = unlockTime as only 15 days are left on the lock.
  5. the unlock Time would be set to : lockedTokens[msg.sender] [tokenContract].unlockTime = lastLockTime + uint32(_duration) = lastLockTime + 15days so he is able to unlock only after 15 days from when he locked .
  6. Bob is able to call unlock because his unlockTime is 15 days after when he locked.
  7. Bob can lock again and gain another NFT doing it for the entire period of the airdrop , being able to lock for only half the duration each time.
  8. this also impact the schnibble bonus as it's based on the lockDuration. which will be 30 days , just before he unlocks it at day 15.

Impact

users are able to unlock their funds in half the time. allowing users to mint NFT's while only locking for minLockDuration/2. this is going to let users be able to lock and unlock more times and mint more nft's. also impacting schnibble bonus.

Tool used

Manual Review

Recommendation

- if (uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime)

+ if (uint32(lastLockTime) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime)

Assessed type

Math

Lines of code

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

Vulnerability details

Summary

The protocol utilizes five price feeds to establish consensus on the USD price of an asset. To determine the current reported price, these feeds can either disapprove or approve a proposed price. Upon reaching a predefined threshold, a disapproved price is removed, whereas an approved price is accepted. However, due to a missing validation check in the approveUSDPrice function, a single price feed can concurrently disapprove and approve the same price proposal.

Vulnerability Detail

there are 5 price Feed roles in the protocol that push the price of tokens supported by the protocol in USD, they approve and disapprove using functions approveUSDPrice and disapproveUSDPrice.

• approveUSDPrice

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

        ...

• disapproveUSDPrice

function disapproveUSDPrice(
        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.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

as we can see in the disapproveUSDPrice it is checked that the msgSender did not previously approve or disapprove the price. but this check is lacking in the approveUSDPrice. allowing a priceFeed to both approve and disapprove a usd price.

Impact

a price Feed role is allowed to approve and disapprove a price , leading to wrong threshold calculation.

Tool used

Manual Review

Recommendation

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.disapprovals[msg.sender] == _usdProposalId)
+            revert ProposalAlreadyDisapprovedError();

Assessed type

Context