Lines of code

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

Vulnerability details

Description

This has been reported by 4naly3er as M-3 and M-4, but the impact would be more severe than Medium, which is why I still submit this report providing richer details about the issue.

Furthermore, considering ERC20 which doesn't on revert on failure is in scope according to the README contest.

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

Whenever a player wants to lock their token they will call LockManager::lock. The current implementation present a major flaw for players who wants to lock ERC20 token, as the contract is using transferFrom to transfer those tokens and is not checking the returned value to detect if the transfer was a failure or not, and in such case for token that doesn't revert on transfer failure, the flow will just continue which will cause of lot of damage in the current application, which seems to warrant High severity.

Impacts

Player can steal others players ERC20 from the contract. (lock using transferFrom)
Player who unlock might not get all their token back. (unlock using transfer)

PoC

Here is a example of Impact #1. how Bob will steal Alice's ZRXToken token (which doesn't revert on transfer failure).

Alice call lock for an amount of 100 ZRX token.
Bob decide to steal Alice's funds (but that could be all the ZRX token holding by the contract)
- He first call setLockDuration with 1 second (as reported by my other issue, since minimum duration can be bypass, that will work)
- Then he call lock with 100 ZRX but with doesn't own any in reality, but as you can see, the function will simply return false without reverting.
```
function transferFrom(address _from, address _to, uint _value)
public
returns (bool)
{
uint allowance = allowed[_from][msg.sender];
if (balances[_from] >= _value
    && allowance >= _value
    && balances[_to] + _value >= balances[_to]
) {
    balances[_to] += _value;
    balances[_from] -= _value;
    if (allowance < MAX_UINT) {
        allowed[_from][msg.sender] -= _value;
    }
    Transfer(_from, _to, _value);
    return true;
} else {
    return false;
}
}
```
- So the contract will account that Bob now has also 100 ZRX token locked, same as for Alice (but it's not true, the contract still only hold Alice's token)
Since Bob have a duration of only one second, he can submit another transaction almost right away to unlock, effectively stealing the token from Alice.

For Impact #2, the flaw would cause a user fund loss has if the contract doesn't hold enough token (and the transfer fails), the transaction will be successful but the player will receive zero token while his claim will be gone as lockedToken.quantity would be set.

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

Recommended Mitigation Steps

Use safeTransfer()/safeTransferFrom() instead of tranfer()/transferFrom() to transfer ERC20 token so in case of transfer failure, transaction will revert.

Assessed type

ERC20

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

Contract doesn't handle ERC20 token which `don't revert on failure` which cause user fund loss #532