The missing assertion of a boolean return value of `transferFrom` allows draining the whole market of a token if the token doesn't revert on failures, but just returns "false"

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Due to a missing boolean return's value assertion check in the transferFrom function, if the "doesn't-revert-on-failure" tokens are enabled in the protocol, then the missing transferFrom return's check can be utilized further in the lock function to inflate the account's quantity property in the lockedTokens[msg.sender] storage, increasing its quantity infinitely and with no expenses whatsoever (except for gas fees, of course).

→ This creates a dangerous situation: the contract tracks the tokens as deposited, yet no funds were in fact charged from the user's account due to errors that happened during the transferFrom ERC20 execution.

Impacts:

• For instance, the msg.sender's ERC20 account may have insufficient funds, or the allowance on the LockManager's behalf may be too low to spend the msg.sender's tokens. If the user is malicious, he can utilize this to drain the funds of other users that are locked on their behalfs in the LockManager contract;
• potentially, this vulnerability can escalate both as an intended attack vector, and as an unexpected invariant that will add more quantity than the user expects if (it can be a fair, normal user, too!!) the user's account has too little funds for the deposit lock.

Miscellaneous:

1. It's important to note that the protocol's docs here state that the "doesn't revert on failure" ERC20 tokens ARE CONSIDERED IN-SCOPE;
2. Although the 4naly3er's report does mention missing return value assertions, but it marked that as a Medium severity, although it's clearly a High one.

Proof of Concept

1. Update the [src/tokens/]TestERC20Token.sol contract in the following manner:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.25;
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "../interfaces/IBlast.sol";

// A bare-bones ERC20 token for use in tests
contract TestERC20Token is ERC20 {
    uint256 numberClaims;
    bool configured;

    constructor() ERC20("TestERC20Token", "TEST") {
        // Mint 100 tokens to msg.sender
        _mint(msg.sender, 100 * 10 ** decimals());
    }

    // rebasing
    function configure(YieldMode) external returns (uint256) {
        configured = true;
        return 0;
    }

+    function transferFrom(address from, address to, uint256 amount) public override returns (bool success) {
+        (success,) = address(this).delegatecall(abi.encodeWithSelector(this.transferFrom.selector, from, to, amount));
+    }

    function claim(address, uint256) external returns (uint256) {
        numberClaims++;
        return 0;
    }

    function getClaimableAmount(address) external pure returns (uint256) {
        return 0;
    }

    function mint(address user, uint256 amount) external {
        _mint(user, amount);
    }
}

2. Run the following commands and see that the lock call in the [tests/managers/LockManager/]unlock.test.ts DOES NOT revert:

Please make sure you add the following update ↓

237: const approveERC20TxHash = await testERC20Contract.write.approve([
238:          testContracts.lockManager.contract.address,
239: -        parseEther("2"),
239: +        parseEther("0"), // do a zero approval instead to make the transferFrom error later
240:       ]);

pnpm install # if not run already
forge install # if not run already
pnpm run build:solidity
pnpm run build:abi
pnpm test

References

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

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

Tools Used

Recommended Mitigation Steps

if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
-            token.transferFrom(_tokenOwner, address(this), _quantity);
+            require(token.transferFrom(_tokenOwner, address(this), _quantity), "ERC20 transfer failed");
        }

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

You can see that the quantity has increased, yet no tokens were in fact transferred from the user.

This can be utilized to run an unfair unlock(address _tokenContract, uint256 _quantity) later, and the user will steal all doesn't-revert-on-failure tokens from the LockManager contract, leaving the contract drained.

Assessed type

ERC20

[c4-judge]

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

[c-plus-plus-equals-c-plus-one]

@alex-ppg why was this out of scope? The LockManager was a listed asset in-scope.