Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L373-L377 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L379-L384

Vulnerability details

Impact

An attacker can call the _lock() function with a very large _quantity() of tokens as argument that would cause transferFrom() to return false but not revert. _lock() function would continue execution and update the attacker's locked balance accordingly. This way attacker can increase their locked balance without actually transferring tokens due to the issue of tokens that do-not-revert-on-failure.

Proof of Concept

According to the protocol'e readme, tokens that does not revert on failure are in scope for this audit contest:

Doesn't revert on failure- Yes

For instance, ZRX is a token that do not revert on failure, instead it simply returns a boolean value that is not checked:

function transferFrom(address _from, address _to, uint _value) returns (bool) {
        if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && balances[_to] + _value >= balances[_to]) {
            balances[_to] += _value;
            balances[_from] -= _value;
            allowed[_from][msg.sender] -= _value;
            Transfer(_from, _to, _value);
            return true;
        } else { return false; }
    }

An attacker can take advantage of this, because the _lock() function uses an outdated token transfer method: LockManager.sol#L373-L377

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

Example Scenario:

Alice attempts to lock 10000 ZRX tokens using the _lock() function.
However, the token transfer fails due to an issue with the token contract (e.g., insufficient funds).
As the transferFrom() method of ZRX token does not revert on failure but returns false, the _lock() function does not detect the transfer failure and continues execution.
Despite the transfer failure, the _lock() function updates Alice's locked balance in the protocol by 10000 tokens.

This way, Alice was able to increase his locked balance in the contract without actually transferring any tokens. LockManager.sol#L379-L384

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

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using safeTransferFrom() from Openzeppelin's library instead of transferFrom() or check its return value and revert if it's false.

Assessed type

ERC20

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

Users can increase their locked balance without actually transferring tokens when no-revert-on-failure tokens are used #526