Munchables - xyz's results

Lines of code

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

Vulnerability details

Impact

One of the main invariants of the project is that users can not reduce lockup times that are previously set, however, due to a business logic error in the setLockDuration function, this is possible.

Proof of Concept

The setLockDuration function validates if the given _duration would lead to a reduced lockup time.

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

The validation adds the block.timestamp to the _duration, and checks if this is reduced compared to the current unlockTime. This validation is followed by the setting of the new unlockTime.

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

However, here the _duration is added to the lastLockTime and not the block.timestamp, which means that the user can reduce the unlockTime at a maximum by the elapsed time between the lastLockTime and block.timestamp. This value will always be miscalculated and will set an invalid unlockTime for every user, not just the ones with bad intentions who want to reduce their unlockTime.

Tools Used

Manual review.

Recommended Mitigation Steps

    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 +
+                   block.timestamp +
                    uint32(_duration);
            }
        }

        emit LockDuration(msg.sender, _duration);
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

approveUSDPrice is missing the validation for ProposalAlreadyDisapprovedError(). This can lead to unintended USD price voting outcomes, as the msg.sender, can make an approved and also a disapproved vote, essentially lowering the threshold for executing a proposal. There is no way to revoke approvals or disapprovals.

Proof of Concept

The disapproveUSDPrice validates if the caller has already approved the USD price.

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

Because of this validation, the caller can not achieve a state with both an approval and a disapproval if first the approveUSDPrice is called and then disapproveUSDPrice, as the disapproveUSDPrice will revert, and the caller will end up only with an approval. However, this state can be achieved if first the disapproveUSDPrice is called and then the approveUSDPrice, as the approveUSDPrice is missing the validation if the msg.sender has already disapproved the USD price.

Tools Used

Manual review.

Recommended Mitigation Steps

Add validation to the approveUSDPrice if the msg.sender has already disapproved the USD price.

    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