Locking native tokens using a non-EOA account makes funds unrecoverable

[c4-bot-2]

Lines of code

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

Vulnerability details

Locking native tokens using a non-EOA account makes funds unrecoverable

Impact

The lock() method allows to lock funds and mint Munchables NFT. The lockOnBehalf() method allows to lock funds on behalf of a player.

If a player uses a smart contract without the receive/fallback methods instead of an EOA to lock his/her native tokens or decides to lock on behalf of a smart contract address, instead of an EOA, he/she will not be able to unlock his/her funds after the unlockTime period. This means that his/her funds will be stuck indefinitely inside the LockManager contract. Furthermore, the call to the unlock method will not revert, decreasing the quantity amount of that player.

This is not the same that was reported in automatic finding M-02 which describe that the transaction could fail due to the gas cost because the root cause is different.

Proof of Concept

Both lock() and lockOnBehalf methods are payable and accept ethers. The quantity of native tokens locked by a player is saved in the lockedTokens variable:

LockManager.sol#L23-L24

/// @notice Player's currently locked tokens, can be multiple. Indexed by player then token contract
mapping(address => mapping(address => LockedToken)) public lockedTokens;

LockManager.sol#L333-L335

LockedToken storage lockedToken = lockedTokens[_lockRecipient][
 _tokenContract
];

LockManager.sol#L380

lockedToken.quantity += _quantity;

After the lock period, they can call the unlock() method:

LockManager.sol#L400-L427

400      /// @inheritdoc ILockManager
401      function unlock(
402          address _tokenContract,
403          uint256 _quantity
404      ) external notPaused nonReentrant {
405          LockedToken storage lockedToken = lockedTokens[msg.sender][
406              _tokenContract
407          ];
408          if (lockedToken.quantity < _quantity)
409              revert InsufficientLockAmountError();
410          if (lockedToken.unlockTime > uint32(block.timestamp))
411              revert TokenStillLockedError();
412  
413          // force harvest to make sure that they get the schnibbles that they are entitled to
414          accountManager.forceHarvest(msg.sender);
415  
416          lockedToken.quantity -= _quantity;
417  
418          // send token
419          if (_tokenContract == address(0)) {
420              payable(msg.sender).transfer(_quantity);
421          } else {
422              IERC20 token = IERC20(_tokenContract);
423              token.transfer(msg.sender, _quantity);
424          }
425  
426          emit Unlocked(msg.sender, _tokenContract, _quantity);
427      }

Thanks to line 416, the lockedToken.quantity is decreased by the number of native tokens requested in the _quantity variable. However, the line 420 will not work in the right way attempting to transfer native tokens to a non-payable contract.

Even if this issue is due to a wrong implementation of the player locker contract, the fact that the quantity value is decreased even if the value is not transferred is unfair and creates an inconsistency between the lockedToken.quantity and the actual locked amount (the value returned by the getLocked() method is incorrect).

Tools Used

Visual inspection

Recommended Mitigation Steps

We propose to allow player choosing where LockManager has to send unlocked funds:

LockManager.sol#L400-L427

 400      /// @inheritdoc ILockManager
 401      function unlock(
 402          address _tokenContract,
 403          uint256 _quantity,
+                address recipient
 404      ) external notPaused nonReentrant {
 405          LockedToken storage lockedToken = lockedTokens[msg.sender][
 406              _tokenContract
 407          ];
 408          if (lockedToken.quantity < _quantity)
 409              revert InsufficientLockAmountError();
 410          if (lockedToken.unlockTime > uint32(block.timestamp))
 411              revert TokenStillLockedError();
 412  
 413          // force harvest to make sure that they get the schnibbles that they are entitled to
 414          accountManager.forceHarvest(msg.sender);
 415  
 416          lockedToken.quantity -= _quantity;
 417  
+                if (recipient == address(0)) recipient = msg.sender;
 418          // send token
 419          if (_tokenContract == address(0)) {
-   420              payable(msg.sender).transfer(_quantity);
+                    payable(recipient).transfer(_quantity);
 421          } else {
 422              IERC20 token = IERC20(_tokenContract);
-   423              token.transfer(msg.sender, _quantity);
+                    token.transfer(recipient, quantity);
 424          }
 425  
-   426          emit Unlocked(msg.sender, _tokenContract, _quantity);
+                emit Unlocked(recipient, __tokenContract, _quantity);
 427      }

Assessed type

ERC20

[niser93]

This issue was judges as unsatisfactory because it is due to a user mistake and it was reported by bot M-2. However, in M-2 the bot reported the gas cost as root cause:

The use of the deprecated transfer() function for an address may make the transaction fail due to the 2300 gas stipend

The issue I reported is different because the transaction doesn't fail: it succeeds and transfers an amount of native tokens. However, when an user asks to unlock that funds, he/she would fail. This behavior is very unfair and it is not due to a user mistake. Why an user should not use a non-EOA address to lock his/her funds? There is not documentation that advise against this behavior. I think it is a standard behavior to use a non-EOA addresses.

So, we think that the following behavior is very unfair: a user chooses to use a non-EOA address to lock funds on Munchables. After at least 30 days, the user discovers his/her funds are unrecoverable.

I want to add a very important thing: in the future, no contracts will be able to unlock their funds on the Munchables protocol. Even other protocol's contracts will not be able to do that. And we think this will be a problem. Let's think, for example, to a vault that aims to collect funds in order to mint Munchables, for example. It will be able to lock funds, but not to unlock them.

Could you please evaluate this issue again? Thank in advance

[alex-ppg]

Hey @niser93, the submission has been graded by the validators, and did not receive judge feedback. Smart contracts not having a receive or fallback payable hook to accept native funds are the ones responsible for interacting with the contract securely.