Lines of code

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

Vulnerability details

Impact

If the token send fails, the player may loss locked tokens

Proof of Concept

OpenZeppelin’s IERC20 interface are expected to return a bool value after a successful transfer:

openzeppelin/contracts/token/ERC20/IERC20.sol

    function transfer ( address to , uint256 amount ) external returns ( bool );

Also, in the ReadMe file of Code4Rena Audit, it is mentioned that more ERC20 tokens to be used in LockManager will be added in the future.

Scoping Q & A: ERC20 used by the protocol: USDB, WETH, (assume we can add more to the future for use in LockManager)

Based on the above facts, certain assets such as ZRX can be added to LockManager. These specific tokens are not reverted and return false if token transmission fails. However, the LockManager.sol#unlock() function does not check the return value if token send fails.

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

416:    lockedToken.quantity -= _quantity;

        // send token
        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);
        } else {
            IERC20 token = IERC20(_tokenContract);
423:        token.transfer(msg.sender, _quantity);
        }

        emit Unlocked(msg.sender, _tokenContract, _quantity);
    }

Some ERC20 tokens are not reverted when token send fails and return a bool value, so the unlock() function is not reverted. Therefore, if token send fails, Locked assets corresponding to _quantity cannot be unlocked, and the assets may be locked in the contract forever by #L423.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using safeTransfer consistently instead of transfer.

    function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        ... ...
        else {
            IERC20 token = IERC20(_tokenContract);
---         token.transfer(msg.sender, _quantity);
+++         token.safeTransfer(msg.sender, _quantity);            
        }
        ... ...
    }

Assessed type

Token-Transfer

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

Use safeTransfer consistently instead of transfer #527