Munchables - Walter's results

Lines of code

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

Vulnerability details

Impact

The lockOnBehalf function allows an attacker to repeatedly force a harvest operation on a victim user by passing zero ETH and zero quantity to a lockOnBehalf call where the OnBehalfOf is the victim. This results in a Denial-of-Service (DoS) attack that prevents the victim from accumulating Schnibbles rewards over time. in fact forceHarvest must be called only from wallets authorized by the user who is involved in the harvest, not just anyone can do it, otherwise this problem arises also because inside (_harvest) there is a division for /1 days which ultimately helps this problem making the rounding down.

Proof of Concept

1. The attacker calls the lockOnBehalf function with the following parameters:

    _tokenContract: address(0)
    _quantity: 0
    _onBehalfOf: Address of the victim user
    msg.value: 0

hypothetical transaction (could be tested in the current suit):

it("should lock token for other player", async () => {
        const { request } = await testContracts.lockManager.contract.simulate.lockOnBehalf(
          [zeroAddress, parseEther("0"), alice],
          { account: bob, value: parseEther("0") }
        );
        const txHash = await testClient.writeContract(request);
        await assertLockSuccess({
          testContracts,
          testERC20Contract,
          numberNFTs: 0n,
          player: alice,
          quantity: parseEther("0"),
          lockedQuantity: parseEther("0"),
          remainder: 0n,
          sender: bob,
          tokenContractAddress: zeroAddress,
          txHash,
        });
      });

2. This call goes through the following steps:
   1. The lockOnBehalf function checks the parameters and calls the _lock function.
   2. In _lock, the accountManager.forceHarvest(_lockRecipient); line forces a harvest for the victim.
   3. The _harvest function calculates the Schnibbles to be harvested based on the elapsed time since the last harvest. Since the attacker makes frequent calls (like one every few hours), the elapsed time is very short, and the rewards are negligible (rounded to zero).
   4. The lastHarvestDate of the victim is updated, preventing the accumulation of Schnibbles rewards over time.

repeat this process every few hours,depending on how much the victim has in the protocol(getDailySchnibbles)

Tools Used

manual review

Recommended Mitigation Steps

add an allowance mapping that tracks every delegation,like so: mapping(address account => mapping(address spender => bool)) private _allowances; and add the checks as a modifier like so:

    modifier onlyAuthorized(address onBehalfOf){
        if (!_allowances[onBehalfOf][msg.sender])revert NotAuthorized();
        _;
        }

that allows only authorized parties to lock tokens on behalf of other users.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Users can exploit this vulnerability to reduce the lock duration of their tokens, allowing them to unlock tokens earlier than intended. This undermines the lock mechanism and allows users to receive rewards without adhering to the proper lock time, potentially leading to economic imbalances and unfair advantages.

Proof of Concept

Assume the following variables:

    currentTimestamp = 100
    (attackCall) _duration = 30
    lastLockTime = 80 (some point in the past where user locked tokens)
    pastDuration = 50 (original duration of the locked token at 80 of timestamp,lastLockTime)
    currentUnlockTime = lastLockTime + pastDuration = 80 + 50 = 130

If the current unlockTime is, for example, 130, this check will pass:

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

in fact this if statement will be: 130(100+30) < 130 => false = skip

and lastly will be reduced the unlockTime at this line:

lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration); // 80 +30 = 110 @audit reduced!

reducing in this way the unlockTime!

will leave the function commented to get a better idea:

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) < // 100 + 30 @audit - MEDIUM needs to check lastLockTime + duration lower than current unlockTime
                    lockedTokens[msg.sender][tokenContract].unlockTime // 130
                ) {
                    revert LockDurationReducedError();
                }

                uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
                    .lastLockTime;
                lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration); // 80 +30 = 110 @audit reduced!
            } // users can reduce duration of timelock
        }
        emit LockDuration(msg.sender, _duration);
    }

Tools Used

manual review

Recommended Mitigation Steps

2 options:

1. change this line:

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

to this:

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

2. change this if statement:

                if (
                    uint32(block.timestamp) + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                )

to this

                if (
                    lockedTokens[msg.sender][tokenContract]
                    .lastLockTime + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                )

Assessed type

Context

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

4

Missing Check for Prior Disapproval in approveUSDPrice Function

The approveUSDPrice function allows price feed contracts to vote in favor of a proposed price. However, unlike the disapproveUSDPrice function, approveUSDPrice does not verify whether the voter has already voted against the current proposal. A price feed contract should only be allowed to vote on one side (approve or disapprove) of the proposal. This missing check can lead to inconsistent voting behavior and potential conflicts in the proposal process.

Affected code

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();
    // @audit - LOW should check even disapprovals
    if (usdUpdateProposal.proposedPrice != _price)
        revert ProposalPriceNotMatchedError();

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

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

    emit ApprovedUSDPrice(msg.sender);
}

LockManager#177-207

Potential Impact

Without a check to prevent a voter who has disapproved the proposal from approving it, the voting process can become inconsistent, leading to potential manipulation or invalid results. This could undermine the integrity of the price update mechanism.

Recommended Fix

Add a check in the approveUSDPrice function to ensure that a voter who has already disapproved the proposal cannot approve it.

1

Missing Validation Checks for decimals and usdPrice in configureToken Function

A potential issue has been identified within the configureToken function related to the validation of token data. Specifically, there is no check to ensure that the decimals value in the _tokenData parameter is different from 0, and that the usdPrice is equal to 0.

This is important because if decimals is 0 and usdPrice is different from 0, the price will later be modified by the price feed, which may lead to unexpected behavior or incorrect pricing.

affected code:

function configureToken(
    address _tokenContract,
    ConfiguredToken memory _tokenData
) external onlyAdmin {
    if (_tokenData.nftCost == 0) revert NFTCostInvalidError();
    // @audit - add here all the validation checks
    if (configuredTokens[_tokenContract].nftCost == 0) {
        configuredTokenContracts.push(_tokenContract);
    }
    configuredTokens[_tokenContract] = _tokenData;

    emit TokenConfigured(_tokenContract, _tokenData);
}

LockManager#115-127

Recommended fix

Add checks to ensure that the decimals field in _tokenData is not 0 and that the usdPrice is 0 before assigning _tokenData to configuredTokens[_tokenContract].

2

Missing Validation Checks for setUSDThresholds Function

The setUSDThresholds function is intended to set the thresholds for approvals and disapprovals of a price by the price feed contracts. However, the function lacks validation checks for the _approve and _disapprove parameters. Specifically, these values must be between 2 and 5 (inclusive). If the thresholds are not properly validated, it can lead to invalid configurations that disrupt the voting process.

Code Snippet

function setUSDThresholds(
    uint8 _approve,
    uint8 _disapprove
) external onlyAdmin {
    if (usdUpdateProposal.proposer != address(0))
        revert ProposalInProgressError();
    // @audit approve and disapprove need to be >= 2 and <= 5
    APPROVE_THRESHOLD = _approve;
    DISAPPROVE_THRESHOLD = _disapprove;

    emit USDThresholdUpdated(_approve, _disapprove);
}

LockManager#129-139

Potential Impact

Without proper validation, thresholds could be set to values that are either too low or too high. If the threshold is set to 1, it undermines the voting process, as the proposer is always counted as one vote. And for the fact that in the propose function there's no if statement that checks approvals,this will skip the first vote,making the first effective valide threshold at 2.

Recommended Fix

Add checks to ensure that the _approve and _disapprove values are between 2 and 5 inclusive. This ensures that the voting process is not compromised by invalid threshold values.

3

Unnecessary Deletion of usdUpdateProposal in proposeUSDPrice Function

The proposeUSDPrice function is responsible for proposing a new price for an asset by the price feed contracts. Within this function, the usdUpdateProposal struct is instantiated to temporarily store the proposed values. This struct is used throughout the proposal process and is either approved or disapproved through separate functions (approveUSDPrice and disapproveUSDPrice). When a proposal reaches a threshold for approval or disapproval, the proposal is either executed or discarded, and the struct is reset to make way for new proposals.

The issue is that the line delete usdUpdateProposal; is unnecessarily present at the beginning of the proposeUSDPrice function. This deletion is redundant because the usdUpdateProposal struct will be reset regardless of whether the proposal is approved or disapproved. Therefore, this line of code does not contribute to the function's purpose and can be removed to clean up the code.

Code Snippet

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; // @audit QA no need to delete 

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

LockManager#142-174

Potential Impact

While this issue does not affect the functionality or security of the contract, it represents unnecessary code that could be removed to improve readability and maintainability.

Recommended Fix

Remove the line delete usdUpdateProposal; from the proposeUSDPrice function to eliminate redundant code.

4

Missing Check for Prior Disapproval in approveUSDPrice Function

The approveUSDPrice function allows price feed contracts to vote in favor of a proposed price. However, unlike the disapproveUSDPrice function, approveUSDPrice does not verify whether the voter has already voted against the current proposal. A price feed contract should only be allowed to vote on one side (approve or disapprove) of the proposal. This missing check can lead to inconsistent voting behavior and potential conflicts in the proposal process.

Affected code

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();
    // @audit - LOW should check even disapprovals
    if (usdUpdateProposal.proposedPrice != _price)
        revert ProposalPriceNotMatchedError();

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

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

    emit ApprovedUSDPrice(msg.sender);
}

LockManager#177-207

Potential Impact

Without a check to prevent a voter who has disapproved the proposal from approving it, the voting process can become inconsistent, leading to potential manipulation or invalid results. This could undermine the integrity of the price update mechanism.

Recommended Fix

Add a check in the approveUSDPrice function to ensure that a voter who has already disapproved the proposal cannot approve it.

5

Users can bypass minLockDuration check

The setLockDuration function allows setting the lock time for a specific asset locked in the contract. While it correctly checks that the provided duration is less than the MaxLockDuration, it does not ensure that the duration is greater than the MinLockDuration. This omission can lead to lock durations that are too short, bypassing the intended minimum lock period.

function setLockDuration(uint256 _duration) external notPaused {
    if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
        revert MaximumLockDurationError();
    // @audit - LOW - need to add minLockDuration this could bypass minLockDuration check (present in lock,but not here)

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

LockManager#245-272

Potential Impact

Failing to enforce a minimum lock duration can allow users to set lock times that are too short, which might not comply with the intended locking policies. This can lead to issues where assets are unlocked too soon, defeating the purpose of having a minimum lock duration.

Recommended Fix

Add a check to ensure that the _duration is also greater than or equal to the MinLockDuration.

6

Missing Check for active Status in _lock Function

The _lock function allows locking tokens within the contract, but it lacks a check for the active status of the ConfiguredToken. According to the provided documentation in the codebase:

    /// @notice Struct holding details about tokens that can be locked
    /// @param usdPrice USD price per token
    /// @param nftCost Cost of the NFT associated with locking this token
    /// @param decimals Number of decimals for the token
    /// @param active Boolean indicating if the token is currently active for locking
    struct ConfiguredToken {
        uint256 usdPrice;
        uint256 nftCost;
        uint8 decimals;
        bool active;
    }

ILockManager#17-27

The active status should be verified before allowing tokens to be locked. However, this check is missing from the _lock function, which may allow users to lock tokens that are not intended to be locked according to the contract's configuration.

Potential Impact

Without verifying the active status of the token, the _lock function may allow tokens to be locked even if they are not currently intended to be locked according to the contract's configuration. This could lead to unexpected behavior and potential misuse of the locking mechanism.

Recommended Fix

Add a check to ensure that the active status of the ConfiguredToken is true before allowing tokens to be locked in the _lock function.