Munchables - 0xdice91'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#L379-L384 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L401-L427

Vulnerability details

In LockManager.sol the function lockOnBehalf() may be used to lock up assets under another user by setting _onBehalfOf as any user

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

In _lock() the number of new munchables is calculated and assigned to the user, the required asset for lock-up is transferred in and the time in which to token was deposited and the time for withdrawal is recorded and stored

    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();
//More Code...
        // 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
        );
    }

Once the lock period has elapsed users can call unlock() to retrieve their locked tokens, this function checks if the unlocktime has passed before transfers are made, if lockup time has not elapsed the function reverts

    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 issue here is that an attacker brick a user's withdrawal by frontrunning the user's call to unlock and call lockOnBehalf() with the user address and zero or a very tiny amount this will cause _lock() to reset the users unlocktime to a new time in the future, causing the unlock function to revert.

Impact

This will lead to loss of funds to users as this issue is cheap to execute and can be done indefinitely by an attacker.

Tools Used

Manual Review

Recommended Mitigation Steps

A minimum lock amount should be enforced on lockOnBehalf() making it expensive and unprofitable for an attacker to use for bricking withdrawals.

Assessed type

DoS

Lines of code

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

Vulnerability details

According to the contest overview page;

The most important thing is that funds cannot get locked forever, people cannot take other people's funds, and people cannot reduce lockup times that are previously set. People cannot reduce lockup times that are previously set.

The function setLockDuration() is used to set or change the lock up duration, the function also updates all existing lock of the user, it ensures that the current duration is not before current unlocktime.

    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 issue here is that the check in line 257 uses block.timestamp instead of the the lastLockTime of the Lock. This can lead to a situation where once enough time has passed, the user sets the new duration to some time after the unlockTime which will pass the check, then in line 265 this new shorter duration plus the lastLockTime will be set as the unlockTime.

NB: Please refer to the POC section for more understanding

Impact

Users can abuse this to reduce lockup times that are previously set which goes against the intentions of the devs and enables these malicious users to withdraw their lockup assets before time.

Proof of Concept

Note that these values are assumptions to explain the issue

• block.timestamp = 115700 and lets say 100 units = 1 day
• Alice locks up her asset with a duration of 30 days meaning lastLockTime = 115700 and unlockTime = 118700 ((30 days * 100) + block.timestamp)
• After 15 days block.timestamp is now 117200
• Alice calls setLockDuration(1800) that is changing the duration to 18 days
• The check in 257 uses block.timestamp + new duration to check if less than unlockTime revert. The check will pass as (117200 + 1800 = 119000) is > than than 118700 (unlockTime).
• New unlock time is now set to lastLockTime + new duration (115700 + 1800) = 117500 which is now 18 days after locking
• Therefore Alice has reduced the lockup times that are previously set from 30 days to 18 days.

Tools Used

Manual Review

Recommended Mitigation Steps

The setLockDuration() function should be corrected to use the lastLockTime instead of block.timestamp in the check.

   /// @inheritdoc ILockManager
    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;

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

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

        emit LockDuration(msg.sender, _duration);
    }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

The approveUSDPrice() function allows price feed role holders to approve prices after disapproving as there is no check for it the caller has already voted to disapprove the role

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


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


        emit ApprovedUSDPrice(msg.sender);
    }

This breaks the whole system of voting as voters are required to pick a side and not support both sides

Impact

price feed role holders can vote to approve a price after rejecting or declining the proposed price thereby filling up both sides of the threshold and preventing other role holders from voting.

Tools Used

manual review

Recommended Mitigation Steps

The approveUSDPrice() function should be corrected to prevent role holder that have already disapproved the price from calling 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.disapprovals[msg.sender] == _usdProposalId)
++          revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        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

Other