howlbot-integration[bot] commented 1 month ago

Lines of code

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

Vulnerability details

Description

The return value of the message call token.transferFrom() is not checked. This might lead to a serious problem wherein LockManager.sol could not have transferred the tokens to itself but updates the lockedToken data for the user.

Proof of Concept

The following is a detailed step-by-step explanation.

A user X locks a token through the lock() function:

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

    function lock(
        address _tokenContract,
        uint256 _quantity
    )
        external
        payable
        notPaused
        onlyActiveToken(_tokenContract)
        onlyConfiguredToken(_tokenContract)
        nonReentrant
    {
        _lock(_tokenContract, _quantity, msg.sender, msg.sender);
    }

The LockManager.sol in the _lock() function transfers the tokens from the user to itself at LOC-376:

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

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

Let us assume this token (that X locks) does not revert on failure but returns a bool (this token behavior is in scope).

The _lock() function does not check the returned bool. Let us assume that the token transfer (from X to LockManager.sol) failed. However, _lock() function would not be aware of it and set the lockedTokens[X][_tokenContract] data for X as usual (that is, as it would set in the normal conditions when the token transfer would not have failed).

This allows X to enjoy the benefits of token lock without any locking of token in reality.

Moreover, if X calls unlock() function with a positive _quantity, the LockManager.sol would lose tokens to X:

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

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

This is a serious case where the LockManager.sol would lose assets.

Impact

Impact: The issue mentioned above clearly shows that the LockManager.sol contract can lose asset, thus, making this issue a High.

Likelihood: The likelihood remains Medium as, configureToken() function is called by admin.

Tools Used

VS Code Manual review

Recommended Mitigation Steps

It is recommended to add a require statement with the returned bool in the _lock() function at LOC-376:

bool result = token.transferFrom(_tokenOwner, address(this), _quantity);
require(result);

Assessed type

Token-Transfer

c4-judge commented 1 month ago

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

Ys-Prakash commented 1 month ago

Hi @alex-ppg

Thanks a lot for taking the time to judge this contest.

I could not comprehend your decision to mark this issue as OOS. I would like to add the following information:

The issue has an assumption that the protocol would use tokens that do not revert on failure. This assumption seems valid as it is mentioned in the README of the contest:

Question	Answer
Doesn't revert on failure	Yes

Also, the addition of more ERC20 tokens by the protocol makes this assumption more concrete:

Question	Answer
ERC20 used by the protocol	USDB, WETH, (assume we can add more to the future for use in LockManager)

This makes it clear that apart from ETH, WETH, and USDB, the protocol could use tokens that do not revert on failure, that is, this token behavior is in-scope.

The main reason that seems to me behind why this issue has been OOSed is because it is present in the 4naly3er report.

To this, I would like to state the judgment from Inaugural SC session, Fall 2023, from C4 docs.

First of all, the judgment that stated:

When the only prerequisite for a high or medium issue is a Bot Race finding, and fixing the relevant bot race finding in the most logical common way will cause the issue to no longer exist, the issue is invalid.

had been invalidated. This seems to mean that just because the issue in the findings repo and the 4naly3er report share the same root cause, it should not become a ground for invalidation of the issue from the findings repo.

The judgment further stated that:

Because we cannot determine a specific line, we believe that this Ruling should be changed to consultative

For Example:

If the Sponsor ran 4nalyz3r and claimed all the findings as known

Then escalating some severities by chaining would be in scope

This seems to mean that the case, where an issue should be marked as in-scope or OOS when a relatively similar issue exists within the 4nalyz3r report, is subjective. It may happen that the issue mentioned in the 4nalyz3r report does not clearly define the maximal impact of the bug. However, the issue in the findings repo does so. This would generally result in an upgradation of the severity of the issue in the findings repo from that of the issue in the 4nalyz3r report (which is the case here; the issue in the 4nalyz3r report is marked as M and does not define the maximal impact; however, the current issue is a H severity one with clearly defined maximal impact).

Now, please allow me to describe the maximal impact of this issue once again to make it clear what has not been described in the 4nalyz3r report.

The main point of the severity is that LockManager.sol would lose tokens.

Let us assume a user X calls the lock() function to lock a certain amount of a token that does not revert on failure. The _lock() function gets called in the process. Now, At LOC-376, LockManager.sol transfers tokens from the user (here X) to itself:

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

Let us assume this transfer reverts. However, since there is no boolean check here, LockManager.sol would move ahead and set the details of lockedTokens[X][_tokenContract]:

        lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
        lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

This would result in lockedTokens[X][_tokenContract].quantity to increase. However, the tokens are not transferred to LockManager.sol from X.

Now comes the part that the 4nalyz3r report misses. To loot tokens from the LockManager.sol (so that maximal impact is manifested), X would need to call the unlock() function after his unlockTime is over:

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

The accounting takes into account the increased lockedTokens[X][_tokenContract].quantity. Hence, X would be able to receive tokens from LockManager.sol that he did not lock, thereby, looting tokens from LockManager.sol.

Note:

The assumption is made that X calls lock() outside of the LockDrop period. If called within the LockDrop period, X would also be able to reveal NFTs along with gaining the ability to drain tokens from LockManager.sol once his unlockTime gets over.

Thus, this is a clear case of protocol assets lost directly, hence, making a valid case for a High. Now,

The 4nalyz3r report fails to account for this maximal impact.
Since this issue seems to be a High now (which is an upgradation from Medium stated in the 4nalyz3r report) this would make a clear case for being in-scope according to the Inaugural SC session judgment mentioned in the 2nd point above:
- Then escalating some severities by chaining would be in scope

I would be more than happy to provide more valuable input.

Thanks

alex-ppg commented 1 month ago

Hey @Ys-Prakash, I greatly appreciate your PJQA contribution as well as your assistance in debating other submissions! I will maintain the OOS ruling and will defer to an explanation as given in this submission here: https://github.com/code-423n4/2024-05-munchables-findings/issues/467#issuecomment-2158507699

code-423n4 / 2024-05-munchables-findings

Return value of token.transferFrom() not checked #529