Munchables - prapandey031's results

Lines of code

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

Vulnerability details

Description

Users can reduce their lockedTokens[user][tokenContract].unlockTime for a locked token due to a math error in the function setLockDuration(uint256 _duration) function.

Proof of Concept

The following is a detailed step-by-step explanation.

Let us assume user X has set its playerSettings[user].lockDuration to be equal to 50 days through the setLockDuration(uint256 _duration) function:

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

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

Now, X locks certain amount of USDB through the lock() function:

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

    function lock(
        address _tokenContract,
        uint256 _quantity
    )
        external
        payable
        notPaused
        onlyActiveToken(_tokenContract)
        onlyConfiguredToken(_tokenContract)
        nonReentrant
    {
        _lock(_tokenContract, _quantity, msg.sender, msg.sender);
    }

The _lock() function sets the following in LOC-:381-384:

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

Now, after 25 days passed since X locked tokens, X again calls setLockDuration(uint256 _duration) with _duration == 25 days.

The checks are handled as MaxLocDuration == 90 days (> _duration).

Since X has still USDB locked, its unlockTime will be configured.

The check at LOC-256 is easily tackled as block.timestamp + 25 days == lockedTokens[X][tokenContract].unlockTime (as mentioned, this happens after 25 days passed since locking).

The interesting thing happens at LOC:-263-267, where:

lastLockTime == lockedTokens[X][tokenContract].lastLockTime == block.timestamp - 25 days

lockedTokens[X][tokenContract].unlockTime = lastLockTime + 25 days = block.timestamp - 25 days + 25 days = block.timestamp

Thus, X is successful in reducing the unlock time by half.

Now, X would call the unlock() function:

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

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

Since, now lockedToken.unlockTime == block.timestamp, the checks would be easily tackled and X would be able to unlock their USDB tokens 25 days prior.

The root cause is wrong math in LOC-:265-267:

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

block.timestamp should be used instead of lastLockTime.

Impact

Impact: A serious core invariant is broken.

The contest README states: Attack ideas (where to focus for bugs): The most important thing is that funds cannot get locked forever, people cannot take other people's funds, and that people cannot reduce lockup times that are previously set.

The protocol loses tokens before the actual unlock date arrives. This makes it a case for High.

Likelihood: Since the functions are available to any user, the likelihood is also High.

Note

The function setLockDuration(uint256 _duration) does not want the above mentioned behavior. This is clear from the comment in LOC-255:

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

                // check they are not setting lock time before current unlocktime
                if (
                    uint32(block.timestamp) + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                ) {
                    revert LockDurationReducedError();
                }

There is a check to make sure that the new unlock time should not be set before the current unlock time.

Tools Used

VS Code Manual review

Recommended Mitigation Steps

The correct math in LOC-:265-267 should be:

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

It is recommended to correct the math as shown in the abobe mentioned LOCs.

Assessed type

Math

Lines of code

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

Vulnerability details

Description

A single Role.PriceFeed_X (where X belongs to {1, 2, 3, 4, 5}) can 1st disapprove a proposal and then approve the same proposal.

Proof of Concept

The following is a step-by-step explanation of the issue.

Consider a scenario where Role.PriceFeed_1 proposes a USD price proposal through the proposeUSDPrice() function:

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

    function proposeUSDPrice(
        uint256 _price,
        address[] calldata _contracts
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer != address(0))
            revert ProposalInProgressError();
        if (_contracts.length == 0) revert ProposalInvalidContractsError();

        delete usdUpdateProposal;

        // Approvals will use this because when the struct is deleted the approvals remain
        ++_usdProposalId;

        usdUpdateProposal.proposedDate = uint32(block.timestamp);
        usdUpdateProposal.proposer = msg.sender;
        usdUpdateProposal.proposedPrice = _price;
        usdUpdateProposal.contracts = _contracts;
        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

        emit ProposedUSDPrice(msg.sender, _price);
    }

Now, consider the following steps:

1. Role.PriceFeed_2 decides to disapprove the proposal.
2. He calls the disapproveUSDPrice() function to do so:

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

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

        usdUpdateProposal.disapprovalsCount++;
        usdUpdateProposal.disapprovals[msg.sender] = _usdProposalId;

        emit DisapprovedUSDPrice(msg.sender);

        if (usdUpdateProposal.disapprovalsCount >= DISAPPROVE_THRESHOLD) {
            delete usdUpdateProposal;

            emit RemovedUSDProposal();
        }
    }

3. Now, the disapproveUSDPrice() function checks for the approvals[] mapping to not contain the current _usdProposalId:

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

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

This ensures that Role.PriceFeed_2 could not disapprove after he approved the proposal.

The disapproveUSDPrice() function also sets the disapprovals[] mapping for Role.PriceFeed_2 to the current _usdProposalId:

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

usdUpdateProposal.disapprovals[msg.sender] = _usdProposalId;

This ensures that Role.PriceFeed_2 would not be able to replay the disapproveUSDPrice() function.

One thing to note here is that the disapproveUSDPrice() function does not set the approvals[] mapping for Role.PriceFeed_2, that is, usdUpdateProposal.approvals[msg.sender] (with msg.sender == address of Role.PriceFeed_2) still does not contain the current _usdProposalId.

4. After disapproving, Role.PriceFeed_2 tries to approve the proposal by calling the approveUSDPrice() function:

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

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

The 1st, 2nd, and 4th if statements check are easily tackled since: a) The proposal is still active. b) Role.PriceFeed_2 is not the proposer. c) Role.PriceFeed_2 sets the correct _price.

The check of our interest is the 3rd if statement. This also gets tackled as (mentioned above) the usdUpdateProposal.approvals[Role.PriceFeed_2] would still not contain the current _usdProposalId. Hence, the transaction is not reverted. Finally, usdUpdateProposal.approvals[Role.PriceFeed_2] is set to the current _usdProposalId in LOC-199 and the usdUpdateProposal.approvalsCount is increased by 1:

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

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

Hence, Role.PriceFeed_2 was able to first disapprove and then approve a proposal.

The vice-versa cannot happen, that is, first approving and then disapproving cannot happen.

Note

This should not be seen as a facility for the Role.PriceFeed_Xs to rectify their mistake, that is, giving them a chance to approve a proposal that they earlier disapproved by mistake due to the following reasons:

1. There is no such "facility" for the vice-versa case.
2. The usdUpdateProposal.disapprovalsCount is not reduced when the Role.PriceFeed_X decides to approve the proposal after disapproving it earlier.

Impact

A single Role.PriceFeed_X would be able to vote 2 times as well as both approve and disapprove a single proposal.

This is an invariant of the protocol that "should not" be broken, making this a clear case for Medium.

The approve/disapprove functionality clearly would want to restrict a single Role.PriceFeed_X from voting multiple times, either only approving (or, disapproving) multiple times, or, doing both in a combination.

This functionality of the protocol is clearly hindered, thus, impacting its availability. This also consolidates this case to be a Medium.

Tools Used

VS Code Manual review of code

Recommended Mitigation Steps

It is recommended to add the following check in the approveUSDPrice() function:

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

Assessed type

Invalid Validation