Higher decimal tokens will DOS the the contract

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

High decimal tokens are in scope of this audit.

The contract does not handle this case properly and this causes the first lock of the user to be locked in the contract forever and all other attempts to make more locks of the same token type will revert. lock(), lockOnBehalf() and unlock() are affected.

Proof of Concept

The issue is inside getLockedWeightedValue():

18 minus 24 will revert (according to the link that is in the readme of this contest, some tokens can have 24 decimals)

Code reference: link

    function getLockedWeightedValue(
        address _player
    ) external view returns (uint256 _lockedWeightedValue) {
        uint256 lockedWeighted = 0;
        uint256 configuredTokensLength = configuredTokenContracts.length;

        for (uint256 i; i < configuredTokensLength; i++) {
            if (
                lockedTokens[_player][configuredTokenContracts[i]].quantity > 0 &&
                configuredTokens[configuredTokenContracts[i]].active
            ) {
                // We are assuming all tokens have a maximum of 18 decimals and that USD Price is denoted in 1e18
@>              uint256 deltaDecimal = 10 ** (18 - configuredTokens[configuredTokenContracts[i]].decimals);

                lockedWeighted +=
                    (deltaDecimal *
                        lockedTokens[_player][configuredTokenContracts[i]]
                            .quantity *
                        configuredTokens[configuredTokenContracts[i]]
                            .usdPrice) /
                    1e18;
            }
        }

        _lockedWeightedValue = lockedWeighted;
    }

The internal call that calls getLockedWeightedValue() under the hood is accountManager.forceHarvest() both present in _lock() and unlock():

function _lock(
    address _tokenContract,
    uint256 _quantity,
    address _tokenOwner,
    address _lockRecipient
) private {
    ...
    // they will receive schnibbles at the new rate since last harvest if not for force harvest
    accountManager.forceHarvest(_lockRecipient);
    ...
}

function unlock(
    address _tokenContract,
    uint256 _quantity
) external notPaused nonReentrant {
    ...
    // force harvest to make sure that they get the schnibbles that they are entitled to
    accountManager.forceHarvest(msg.sender);
    ...
}

Below I outline the path of internal calls from accountManager.forceHarvest() to getLockedWeightedValue():

Code Reference: link

    function forceHarvest(
        address _player
    )
        external
        onlyConfiguredContract2(
            StorageKey.LockManager,
            StorageKey.MunchadexManager
        )
    {
@>      _harvest(_player);
    }

Code Reference: link

    function _harvest(address _caller) private returns (uint256 _harvested) {
@>      (uint256 dailySchnibbles, uint256 bonus) = getDailySchnibbles(_caller);
        dailySchnibbles += ((dailySchnibbles * bonus) / 1e18);
        ...
    }

Code Reference: link

    function getDailySchnibbles(
        address _caller
    ) public view returns (uint256 _dailySchnibbles, uint256 _bonus) {
@>      uint256 weightedValue = lockManager.getLockedWeightedValue(_caller);
        // Arbitrary division here... If we remove it, we just need to make sure we modify level thresholds, & social/pet bonuses
        _dailySchnibbles = (weightedValue / 10);
        _bonus = bonusManager.getHarvestBonus(_caller);
    }

Because _lock and unlock make an internal call to getLockedWeightedValue before making any state changes this allows the user to successfully lock tokens in the contract. Once the quantity struct variable gets updated to be different than 0 the user will no longer be able to call _lock or unlock, resulting in a DOS when using this token again:

Code reference: link

    function getLockedWeightedValue(
        address _player
    ) external view returns (uint256 _lockedWeightedValue) {
        uint256 lockedWeighted = 0;
        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {
            if (
@>              lockedTokens[_player][configuredTokenContracts[i]].quantity > 0 &&
                configuredTokens[configuredTokenContracts[i]].active
            ) {
                // We are assuming all tokens have a maximum of 18 decimals and that USD Price is denoted in 1e18
@>              uint256 deltaDecimal = 10 ** (18 - configuredTokens[configuredTokenContracts[i]].decimals);

                lockedWeighted +=
                    (deltaDecimal *
                        lockedTokens[_player][configuredTokenContracts[i]]
                            .quantity *
                        configuredTokens[configuredTokenContracts[i]]
                            .usdPrice) /
                    1e18;
            }
        }

        _lockedWeightedValue = lockedWeighted;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Make the appropriate changes to getLockedWeightedValue() to handle high decimal tokens accordingly.

Assessed type

Decimal

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope