Munchables - crypticdefense's results

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L382-L384 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L401-L427 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L275-L294 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L311-L398 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L256-L261

Vulnerability details

Impact

LockManager allows users to lock tokens in return for rewards. They can unlock their tokens once their respective unlockTime has passed.

An attacker can permanently lock another user's tokens by calling lockOnBehalf() with 0 amount, updating the innocent user's unlockTime to the next lockDuration.

The attacker can repeat this before the unlockTime has passed, permanently locking the tokens in LockManager.

Proof of Concept

Alice decides to lock 1000e18 USDB with a lockDuration of 1 days:

LockManager.sol#L382-L384

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

After the unlockTime has passed, Alice decides to unlock her tokens by calling LockManager::unlock.

LockManager.sol#L401-L427

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

An attacker can front-run this transaction by calling lockOnBehalf() with _quantity = 0, and _onBehalfOf = Alice's address.

LockManager.sol#L275-L294

    function lockOnBehalf(
        address _tokenContract,
        uint256 _quantity, //@audit 0 amount
        address _onBehalfOf //@audit Alice's address
    )
        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);
    }

LockManager.sol#L311-L398

    function _lock(
        address _tokenContract,
        uint256 _quantity, //@audit 0 amount
        address _tokenOwner, //@audit Address of attacker
        address _lockRecipient //@audit Alice's address
    ) private {
        // check approvals and value of tx matches
        if (_tokenContract == address(0)) {
            if (msg.value != _quantity) revert ETHValueIncorrectError(); //@audit this will not revert if msg.value == _quantity where msg.value and _quantity are both 0
        } else {
            if (msg.value != 0) revert InvalidMessageValueError(); //@audit if msg.value != 0, we revert because that means the user accidentally sent ETH when they intend to send ERC20 token.
            IERC20 token = IERC20(_tokenContract); //@audit these checks will pass if _quantity is 0, because allowance is 0 as well.
            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 unlockTime is updated with the original lock duration plus the current timestamp

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

As we can see, the allowance checks will pass if 0 amount is specified as the quantity parameter. The unlockTime will be updated to lockedToken.unlockTime = uint32(block.timestamp) + uint32(_lockDuration).

When Alice's unlock() transaction is executed:

LockManager.sol#L410-L411

        if (lockedToken.unlockTime > uint32(block.timestamp)) //@audit unlockTime was just updated, so this will be true
            revert TokenStillLockedError();

It will revert. This process can be repeated to permanently lock her tokens.

It's important to note that there is a function setLockDuration() that allows users to change their unlockTime. However, this function cannot reduce their unlockTime, it can only extend it, as per the following check:

LockManager.sol#L256-L261

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

So if Alice attempts to reduce her unlockTime, it will revert.

Tools Used

Manual Review.

Recommended Mitigation Steps

Revert if 0 amount is specified as quantity:

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
+       if(_quantity == 0) revert zeroAmount();

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L245-L269 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L382-L384 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L256-L261

Vulnerability details

Impact

LockManager allows users to lock tokens in return for rewards. They can unlock their tokens once their respective unlockTime has passed.

LockManager::setLockDuration() allows any user to update their unlockTime if they decide to extend the time to keep their tokens locked.

An attacker can cause DoS of setLockDuration() by updating the user's unlockTime by locking on behalf of them with 0 amount.

Proof of Concept

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) {
                if (
                    uint32(block.timestamp) + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime //@audit they cannot reduce unlockTime
                ) {
                    revert LockDurationReducedError();
                }

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

        emit LockDuration(msg.sender, _duration);
    }

If lockedTokens[msg.sender][tokenContract].quantity > 0, that means the user already has locked some tokens, and if they attempt to reduce their unlockTime, the function will revert.

An attacker can front-run this transaction and call lockOnBehalf with quantity = 0 and specify _onBehalfOf as the address of the user.

Their unlockTime will be updated:

LockManager.sol#L382-L384

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

If the user's _duration specified is less than block.timestamp + _lockDuration, their setLockDuration() call will revert.

LockManager.sol#L256-L261

 if (
    uint32(block.timestamp) + uint32(_duration) <
    lockedTokens[msg.sender][tokenContract].unlockTime //@audit they cannot reduce unlockTime
    ) {
        revert LockDurationReducedError();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Revert if 0 amount is specified as quantity:

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
+       if(_quantity == 0) revert zeroAmount();

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L381-L384 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L256-L269

Vulnerability details

Impact

LockManager allows users to lock tokens in return for rewards. They can unlock their tokens once their respective unlockTime has passed.

setLockDuration() allows users to update their lockDuration, which will also update their unlockTime. Users are only allowed to specify a lockDuration that extends their unlockTime, not one that reduces it.

However, the unlockTime is incorrectly updated from the lastLockTime, rather than from the current unlockTime. This allows a user to reduce their unlockTime, breaking a protocol invariant.

Proof of Concept

When users lock their tokens, the following values are set:

LockManager.sol#L381-L384

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

After the unlockTime has passed, users can unlock their tokens and receive rewards.

During their lock period, users can update their unlockTime by calling setLockDuration() with a new lockDuration

LockManager.sol#L256-L269

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

We revert if uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime (where _duration is the new duration), because a protocol invariant is that users cannot reduce their unlockTime.

The unlockTime is then updated to lastLockTime + uint32(_duration).

Consider the following example:

Alice locks her tokens by calling lock() with the initial _lockDuration of 10 days. lastLockTime is set to the current block.timestamp and unlockTime is set to block.timestamp + 10 days.

Now, let's assume 5 days have passed since she has locked.

Alice calls setLockDuration() with the new _duration as 6 days. The uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime check will not execute because 6 days from the current block.timestamp will be 11 days from the original lock time (since 5 days have already passed). This is greater than the unlockTime, which is 10 days from the original lock time.

To put it more simply:

uint32(block.timestamp) + uint32(_duration) = 6 days from now, which is 11 days after the initial lock (since 5 days have passed).

lockedTokens[msg.sender][tokenContract].unlockTime = 5 days from now, since it was set to 10 days after the initial lock.

So uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime is false, and the unlockTime will be updated.

Recall that lastLockTime is the time when Alice originally locked her tokens.

So the new unlockTime is set to lastLockTime + uint32(_duration), which is 6 days from the original lock date. Alice will now be able to unlock her tokens 6 days from the original lock time (which is 1 day from now, since 5 days have already passed), when she should have only been able to unlock her tokens after 10 days.

In this case, she bypassed the protocol invariant where users cannot reduce lock duration.

Tools Used

Manual Review.

Recommended Mitigation Steps

Update the new _duration from the unlockTime, rather than from the original lock time lastLockTime. Considering the above example, if Alice now specifies 6 days as the new lock duration, it will not change the unlock time from 10 days to 6 days from the original time, but rather 6 more days from the current unlock time, which is total of 16 days from when she originally locked her tokens.

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

+               lockedTokens[msg.sender][tokenContract].unlockTime += _duration;

            }
        }

        emit LockDuration(msg.sender, _duration);
    }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

LockManager allows users to lock tokens in return for rewards. They can unlock their tokens once their respective unlockTime has passed.

When users unlock their tokens, AccountManager::forceHarvest() is called, which gives users reward based off the USD value of their locked tokens.

This USD value can be changed at anytime by a proposer with Pricefeed role via a call to proposeUSDPrice().

If enough Pricefeed roles approve the new USD value, that value will then be updated.

Pricefeed roles can also disapprove the proposed USD value. If enough disapprove, then the proposal will be rejected.

There is a check in place to ensure that if a Pricefeed role has already approved a proposal, then they cannot disapprove it.

However, this invariant can be broken by disapproving first and then proceeding to approve the same proposal.

Proof of Concept

When an address with Pricefeed role disapproves a proposed USD value, there is a check to see if they already approved

LockManager.sol#L225-L226

if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
    revert ProposalAlreadyApprovedError();

This is to ensure that when a proposal is approved by a Pricefeed role, the same address cannot disapprove the same proposal. This is an invariant where the same msg.sender cannot approve and disapprove the same proposal.

However, when approving, there is no check to see if the Pricefeed role has already disapproved the proposal.

#L191-L197

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

The same Pricefeed role can approve a proposal they already disapproved, breaking a protocol invariant.

Tools Used

Manual Review.

Recommended Mitigation Steps

Upon approval, revert if the caller had already disapproved the proposal:

    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();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

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

        if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
            _execUSDPriceUpdate();
        }

        emit ApprovedUSDPrice(msg.sender);
    }

Assessed type

Invalid Validation