Munchables - 0xhacksmithh's results

Lines of code

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

Vulnerability details

Impact

A Normal User funds get locked in contract, beacuse each time an Malicious Attacker can frontrun User's unlock() Tx and lock some DUST amount on behalf him(User's behalf). In result unlockTime for Normal User increased and his unlock() Tx failed.

Proof of Concept

So a user can unlock his locked fund via calling unlock() when unlockTime passed, below is the following code segment

    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)) //@audit
            revert TokenStillLockedError();

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

But this protocol allow Any user to lock for another User via lockOnBehalf()

One major issue here is that there are no minimum amountchecks for _quantity locked

basically a User can call lockOnBehalf() with DUST amount like 1wei for another user's behalf & function flow will like below

lockOnBehalf() --> _lock() https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L275-L294

Whenever a lock happens following state change changes happen

        lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
        lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

Here we see that with each lock, unlockTime set to uint32(block.timestamp) + uint32(_lockDuration) https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L382-L384

here _lockDuration could be playerSettings[_lockRecipient].lockDuration Or if this value not present then lockdrop.minLockDuration

        uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

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

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

So With each DUST lock, unlockTime increased via _lockDuration period

By Following step this attack preformed

• A normal user lock() some fund
• when his unlockTime passed he wanted to withdraw his fund and call unlock()
• Malicious actor see this Tx,
• Malicious actor frontrun User's Tx, and call lockOnBehalf() with parameter _quantity = 1wei(Dust) & _onBehalfOf = User's address
• now _lock() get executed and stateVariables updated, so user's unlockTime now increased block.timestamp + _lockDuration
• User Tx (unlock()) failed due to below check

        if (lockedToken.unlockTime > uint32(block.timestamp))
            revert TokenStillLockedError();

This attack against a Regualr User can lunched multiple times, so basically that user will targeted by an attacker. By doing this an Attacker can keep locking User's fund for infinity time.

Tools Used

Manual review

Recommended Mitigation Steps

To prevent this lockOnBehalf() should have some sort of minimum _quantity check, so that it will become very expensive for an Attacker to keep locking User's fund.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

A User can decrease his lockingPeriod significantly and by pass it via calling setLockDuration() with well crafted _duration parameter. Refer POC for more detail explanation

Proof of Concept

When i talked with sponcer According to him setLockDuration() is used for to increase lock duration But thats not case here. Malicious User can decrese his unlockTime significantly by multiple time calling to setLockDuration() with proper calculated _duration parameter.

Infected code segment here is below

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

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

Lets go with an example to understand this

Some assumption here to keep exmaple crunchy and crispy

• i assume block.timestamp At 50 sec
• & lockdrop.minLockDuration = 50 sec (which will be our lockDuration, cause playerSettings[_lockRecipient].lockDuration = 0 for now)
• lockedToken = lockedTokens[User-S address][ETH]

So now juicy part

• User-S lock 1ETH via lock()

lockedToken.unlockTime = block.timestamp + lockDuration = 50 sec + 50 sec = 100 sec

lockedToken.lastLockTime = block.timestamp = 50 sec

Iteration - 1

• Now 10s passed User-S call setLockDuration(40sec) And above code segment clause checks happen as below

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

if (60 sec + 40 sec < 100 sec) { revert ...}

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

uint32 lastLockTime = lockedToken.lastLockTime = 50 sec lockedTokens.unlockTime = 50 sec + 40 sec = 90 sec

Iteration - 2

1 sec passed block.timestamp at 61sec User-S call setLockDuration(29 sec)

if (61 sec + 29 sec < 90 sec) { revert ...} => FALSE

so STATE update uint32 lastLockTime = lockedToken.lastLockTime = 50 sec lockedTokens.unlockTime = 50 sec + 29 sec = 79 sec

Iteration -3

1 sec passed block.timestamp at 62sec User-S call setLockDuration(17 sec)

if (62 sec + 17 sec < 79 sec) { revert ...} => FALSE

so STATE update uint32 lastLockTime = lockedToken.lastLockTime = 50 sec lockedTokens.unlockTime = 50 sec + 17 sec = 67 sec

So on .....

Here we can see that initial unlockTime for User-S = 100 sec current unlockTime = 67 sec although only 20 sec passed (62 - 50 sec), at 50th second initial lock was created.

This attack could more optimized i just give above simple example to shows root cause and how this works.

Tools Used

Manual Review

Recommended Mitigation Steps

I believe root cause is while upadting unlockTime with new value it is using initial lastLockTime which behave a constant in this situation as it never updated after.

Assessed type

Context

Judge has assessed an item in Issue #546 as 2 risk. The relevant finding follows:

[Low-03] Role.PriceFeed can approve & disapprove same time as there is no check for disapproval in approveUSDPrice()

A PriceFeeder at same time can disapprove and approve same purposed price cause absence of disapproval check in approveUSDPrice()

approveUSDPrice() has following checks

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

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L191-L197 It never checks msg.sender already disapproved current purposal or not, So as a result priceFeeder can first call disapproveUSDPrice() and then call approveUSDPrice() which is not a intended behaviour.

Mitigation

Implement similar check present in disapproveUSDPrice()

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

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L344-L387 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L401-L424

Vulnerability details

Impact

Nft could minted with DUST amount.

Proof of Concept

While User call unlock() it on going to change quantity STATE variable but not consider remainder

    function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        LockedToken storage lockedToken = lockedTokens[msg.sender][
            _tokenContract
        ];
        if (lockedToken.quantity < _quantity) // @audit Remainder not considered
            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; // @audit-issue

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

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

unlock() work as follows

• check _quantity parameter against quantity locked by this User
• Then adjust State of quantity corresponding to input parameter _quantity
• send corresponding amount to User

But Problem here is its not considering to reset or adjust remainder stored for that User

So a User can preform an attack and mint NFT with very less amount (Dust amount) due to above missing check.

For Example ::

Assume a NFT cost = 1 eth

Alice call lock() with 0.9 ETH

Quantity = 0.9 ETH

       uint256 quantity = _quantity + lockedToken.remainder;

remainder = 0.9 ETH

       remainder = quantity % configuredToken.nftCost;

No. Nft minted = 0

        numberNFTs = (quantity - remainder) / configuredToken.nftCost;

So its STATES will be

• remainder = 0.9 ETH
• quantity = 0.9 ETH (we are intersted in these 2. remainder & quantity)
• lastLockTime = block.timestamp
• unlockTime = block.timestamp + lockDuration https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L344-L387

In my Previous Bug Report i explained how this unlockTime could by passed, This attack can preform independently or also with addition of previos one.

Alice call unlock() once unlockTime reached and withdraw all(0.9 ETH)

So its STATES will be

• remainder = 0.9 ETH
• quantity = 0 ETH ... ...

Alice again call lock() with 0.1 Eth

quantity = _quantity + lockedToken.remainder = 0.1 ETH + 0.9 ETH remainder = 0 ETH No. Nft minted = 1

So its STATES will be

• remainder = 0 ETH
• quantity = 0.1 ETH
• lastLockTime = block.timestamp
• unlockTime = block.timestamp + lockDuration

Here we can see Total locked fund for Alice is 0.1ETH and 1 Nft minted for him.

This attack can also go beyond. An attacker can mint Nft with DUST amount by above approch.

Tools Used

Manual review

Recommended Mitigation Steps

While unlocking for an user function should also accounted for remainder stored in storage

If fully unlocking happens then remainder should reset to 0 If partial then remainder adjusted with corresponding unlocking amount

Assessed type

Context