LockedToken Remainder Tracking will be Emptied and Lost When Block.timestamp is not Within Lock Period Range

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

LockedToken Remainder Tracking will be Emptied and completely Lost When Block.timestamp is not Within Lock Period Range as this value is no longer put into consideration in subsequent lock function call which will affect the number of numberNFTs that can be worked with as the remainder value will no longer count.

Proof of Concept

 function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        (
           ...

        // add remainder from any previous lock
        uint256 quantity = _quantity + lockedToken.remainder;
>>>        uint256 remainder; ///@audit Remainder is set to zero since it has been added to quantity above
        uint256 numberNFTs;
        uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

        if (_lockDuration == 0) {
            _lockDuration = lockdrop.minLockDuration;
        }
        if (
            lockdrop.start <= uint32(block.timestamp) &&
            lockdrop.end >= uint32(block.timestamp)
        ) {
            if (
                _lockDuration < lockdrop.minLockDuration ||
                _lockDuration >
                uint32(configStorage.getUint(StorageKey.MaxLockDuration))
            ) revert InvalidLockDurationError();
            if (msg.sender != address(migrationManager)) {
                // calculate number of nfts
  >>>              remainder = quantity % configuredToken.nftCost; ///@audit A new remainder is assigned if Block Timestamp is within Lockdrop Start to End
                numberNFTs = (quantity - remainder) / configuredToken.nftCost;

                if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

                // Tell nftOverlord that the player has new unopened Munchables
                nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
            }
        }

        // Transfer erc tokens
        if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
            token.transferFrom(_tokenOwner, address(this), _quantity);
        }

>>>        lockedToken.remainder = remainder; ///@audit if Block Timestamp is not within Lockdrop Start to End, zero is assigned instead of the old remainder that should have been assigned ❌
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
        lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

        // set their lock duration in playerSettings
        playerSettings[_lockRecipient].lockDuration = _lockDuration;

        emit Locked(
            ...
        );
    }

The function above shows how _lock(...) function is implemented in the LockManager contract, it can be noted from the audit comments and from the pointers in the code above that remainder is first reset to zero at the initial stage, this is because it is added to the quantity that would be later used up to derive numberNFTs in the code, at the second pointer a new remainder is calculated which is then updated as the new remainder in the third and final pointer. The process is only smooth when Block.timestamp is within LockDrop start and end time which is what makes implementation in pointer 2 that handles deriving a new remainder possible. However when Block.timestamp is not within LockDrop start and end time step two is missed, the problem is that instead of reassigning back the remainder as it was not used, zero remainder is instead set. Subsequent lock calls would loss this remainder completely as no tracking is done again

Note: The fact that the remainder was added to quantity at the initial stage does not mean it can be accessible in subsequent calls to derive numberNFTs, as this value is only accessible based on the actual value of remainder state which has already been set to zero

Tools Used

Manual Review

Recommended Mitigation Steps

As adjusted in the code below when block.timestamp is not within start to end lockdrop which means remainder is not used and it should be reassigned again to the old value instead of losing it tracking completely.

 function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        (
           ...

        // add remainder from any previous lock
        uint256 quantity = _quantity + lockedToken.remainder;
        uint256 remainder;
        uint256 numberNFTs;
        uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

        if (_lockDuration == 0) {
            _lockDuration = lockdrop.minLockDuration;
        }
        if (
            lockdrop.start <= uint32(block.timestamp) &&
            lockdrop.end >= uint32(block.timestamp)
        ) {
            if (
                _lockDuration < lockdrop.minLockDuration ||
                _lockDuration >
                uint32(configStorage.getUint(StorageKey.MaxLockDuration))
            ) revert InvalidLockDurationError();
            if (msg.sender != address(migrationManager)) {
                // calculate number of nfts
                remainder = quantity % configuredToken.nftCost;
                numberNFTs = (quantity - remainder) / configuredToken.nftCost;

                if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

                // Tell nftOverlord that the player has new unopened Munchables
                nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
            }
        }
+++     else {
+++         remainder = lockedToken.remainder; ///@audit Since remainder is not used, it should be reassigned again instead of losing it
+++     }
        // Transfer erc tokens
        if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
            token.transferFrom(_tokenOwner, address(this), _quantity);
        }

        lockedToken.remainder = remainder; /// @audit remainder would hold value now
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
        lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

        // set their lock duration in playerSettings
        playerSettings[_lockRecipient].lockDuration = _lockDuration;

        emit Locked(
            ...
        );
    }

Or code should simply revert if it should not be callable when block.timestamp is not within start to end range

Assessed type

Timing

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[Topmark1]

This finding is a duplicate of https://github.com/code-423n4/2024-05-munchables-findings/issues/455 , it should be noted that this report didnt just show that remainder is inaccurately set to zero but it can be noted in the impact that it would affect the number of numberNFTs that can be worked with just as also pointed out in https://github.com/code-423n4/2024-05-munchables-findings/issues/455 most importantly apart from impact and report description both issues offer the same mitigation to ensure storage of remainder is handled properly. based on C4 rule at https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-similar-exploits-under-a-single-issue at "Verdict: Similar exploits under a single issue" heading write up It can be noted from the rule that The findings are duplicates if they share the same root cause. More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates. The two issues have same root cause and same fix and I believe they are duplicate based on C4 rule, thanks for great Judging Effort, kudos

[PetarTolev]

I disagree with the comment above, this issue should not be a duplicate of #455. The important difference is that, although this finding does mention the reset of the reminder, it overlooks the need for the _lock function to be called from the MigrationManager. This is the core problem and represents an essential part of the issue.

[Topmark1]

I respectfully disagree with your comment sir, the very core problem is the remainder reset and this report explains how it relates to LockDrop from the LockManager, which is why the mitigation proposed in this report solves the problem, as noted in the C4 rule, more over the C4 rule is there to guide decisions, I didnt make the rules, I think it's best we leave the final decision for the Judge to make, thanks

[alex-ppg]

Hey @Topmark1 and @PetarTolev, thank you for the discussion under this submission. Submission #454 (this one) is not a duplicate of #455 as it does not describe the migration-specific vulnerability. The vulnerability of the migrator is related to the overwrite of the remainder regardless of the lockdrop period.

The mitigation proposed would not resolve this, as the vulnerable code is within the first if branch and the absence of an else clause in the if (msg.sender != address(migrationManager)) { conditional. As such, this submission is correctly marked as a duplicate of #96 and is ineligible for a reward. I appreciate your contributions, but no further feedback is expected in relation to this submission.