Users can gain rewards with zero quantity

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

Users can gain rewards with zero quantity by unlock method.

Proof of Concept

The condition at lockedToken.quantity < _quantity is checking only if requested _quantity > lockedToken.quantity then it should revert, but users can call unlock method with 0 value of _quantity and earn rewards.

Lets take a look on the code below L401-L427:

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

according to above code there users can pass _quantity as zero value and unlock method will be executed successfully with no revert, that means users can call unlock method every period of time and gain rewards for zero _quantity.

Bug scenario:

• Bob has 10e18 of lockedToken.quantity.
• Bob calls unlock with zero _quantity.
• accountManager.forceHarvest(msg.sender) is done, and calculate the rewards according to lastHarvestDate.
• lockedToken.quantity -= _quantity is done, which lockedToken.quantity will stay the same value.
• Bob can repeat and call unlock method many times, and gain rewards since lockedWeighted is involved of rewards calculation, and relying on the current lockedTokens.quantity.

Tools Used

Manual Review

Recommended Mitigation Steps

Check if _quantity not equal to zero, if it does revert.

Assessed type

Other

[evokid]

please check the scenario of the issue that mentioned in the submission:

Bug scenario:

Bob has 10e18 of lockedToken.quantity. Bob calls unlock with zero _quantity. accountManager.forceHarvest(msg.sender) is done, and calculate the rewards according to lastHarvestDate. lockedToken.quantity -= _quantity is done, which lockedToken.quantity will stay the same value. Bob can repeat and call unlock method many times, and gain rewards since lockedWeighted is involved of rewards calculation, and relying on the current lockedTokens.quantity.

I think the scenario of the bug missed by validators, users can call unlock method with 0 value of _quantity and still earn rewards. and no check for _quantity if equal to zero or not, the impact is not in the favor for the protocol.

[alex-ppg]

Thanks for your feedback @evokid. An unlock of 0 should trigger a reward harvest and is a similar concept adhered to by the likes of Sushiswap etc. staking systems.

There is no security vulnerability arising from it as the Schnibble reward system maintains a distinct time tracking system to calculate how many rewards a user is due.