search

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

0 stars 0 forks source link

Locked tokens may be irretrievable if no-revert-on-failure tokens are used #531

Closed howlbot-integration[bot] closed 1 month ago

howlbot-integration[bot] commented 1 month ago

Lines of code

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

Vulnerability details

Impact

Impact is HIGH. This issue could potentially lead to loss of funds and can make a user unable to unlock their locked tokens.

Proof of Concept

For the scope of this audit, the README states that we can assume more tokens can be added for use in LockManager in future.

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

Also according to the README,

ERC20 token behaviors in scope: Question Answer
Doesn't revert on failure Yes

In the LockManager.sol contract, the transferFrom and transfer function is used without checking its return value:

IERC20 token = IERC20(_tokenContract);
token.transferFrom(_tokenOwner, address(this), _quantity);
 // send token
        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);
        } else {
            IERC20 token = IERC20(_tokenContract);
            token.transfer(msg.sender, _quantity);
        }

Now, ZRX is a token that fits these conditions and can theoretically be used in LockManager as a principal token. https://coinmarketcap.com/dexscan/ethereum/0xfa97aa5002124fe7edd36584628300e8d81df042/

ZRX is a token that does not revert on failure, instead it simply returns a bool value that is not checked.

If _tokenContract is a token that does not revert on failure, this call could fail silently, allowing the rest of the function to execute as if the transfer were successful.

Similarly, BNB just like ZRX does not revert on failure, instead it simply returns a bool value that is not checked too: https://etherscan.io/token/0xB8c77482e45F1F44dE1745F52C74426C631bDD52

Now in the current scope, Unsafe transfers are used in _lock and unlock functions.

Since these attempts to transfer could silently fail, this then leads to multiple scenarios like:

Tools Used

Manual review

Recommended Mitigation Steps:

Ensure that the return value of transferFrom is always checked, and the transaction is reverted if the transfer fails Or use openzeppelin's SafeERC20's library

Assessed type

ERC20

c4-judge commented 1 month ago

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

Rhaydden commented 1 month ago

Hi @alex-ppg

Thank you for your time and effort in judging this contest. I appreciate you. Respectfully, I would like to shed more light on why this issue should not be considered OOS and why it should be considered valid.

First of all, I want to point out that it was explicitly stated in the README that we could assume that more tokens could be added in future:

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

Also, the README stated that:

ERC20 token behaviors in scope:

Question Answer
Doesn't revert on failure Yes

Also, I would like to mention that, although this issue was reported in the 4naly3er as MEDIUM but the impact described in the 4naly3er was vague and is more severe than indicated.

And according to C4 docs,

Wardens may use automated tools as a first pass, and build on these findings to identify High and Medium severity issues ("HM issues").

Given these facts, the report which was submitted highlighted how severe the impact of this issue could be as regards to the fact that we can make assumptions that in future, ZRX can be used in LockManager as a principal token and ZRX is a token that does not revert on failure, instead it simply returns a bool value that is not checked. The same behavior applies to the BNB token.

This issue could lead to a loss of user funds. If the contract lacks sufficient tokens and the transfer fails, the transaction will still be marked as successful. Consequently, the player will receive zero tokens, and their claim will be voided as lockedToken.quantity would be set. These lost tokens would be irretrievable.

Also it is also very risky for the LockManager contract because the contract can also lose assets.

Let's assume a scenario that the BNB token, which Bob locks, does not revert on failure but instead returns a boolean value indicating success or failure.

The _lock() function does not verify the returned boolean value. If the token transfer from Bob to LockManager.sol fails. Despite this failure, the _lock() function would remain unaware and will proceed to update the lockedTokens[Bob][_tokenContract] data for Bob as if the transfer had succeeded.

This flaw allows Bob to reap the benefits of token locking without actually locking any tokens.

Furthermore, if Bob calls the unlock() function with a positive _quantity, LockManager.sol would end up losing tokens to Bob.

I firmly believe this issue should be deemed valid, as addressing it now could be highly beneficial for the protocol, especially if additional ERC20 tokens are integrated in the future.

However, I defer to your judgment and will respect any decision you make on this matter.

alex-ppg commented 1 month ago

Hey @Rhaydden, thanks for your contributions. I am fully aware of the C4 guideline in relation to static-analyzer reported issues, however, I cannot consider an upgrade of one tier (i.e. an M to an H) being a valid submission under this guideline given that H and M risk issues are of equivalent importance in the Sponsor remediating them. Mislabelling a finding as M when it is H is of no actual impact on the Sponsor's remediation actions (i.e. the reason that static analyzer submissions are out-of-scope), and thus my original ruling remains for this finding.