Uneven Gingham Fly - Users might lose funds if WrappedM contract is migrated to a new version.

[sherlock-admin4]

Uneven Gingham Fly

Medium

Users might lose funds if WrappedM contract is migrated to a new version.

Summary

The external call(.transfer) in UsualM::_unwrap function does not have any return value check. So it is assumed that UsualM contract had enough WrappedM balance and the transfer was successful. If WrappedM contract is migrated to a new version where .transfer call returns false upon failure, the recipients will burn their UsualM token but won't get any WrappedM in return.

Root Cause

The UsualM::_unwrap function is as follows:

    /**
     * @dev Unwraps `amount` UsualM from `account` into WrappedM for `recipient`.
     * @param  account   The account from which UsualM is burned.
     * @param  recipient The account receiving the withdrawn WrappedM.
     * @param  amount    The amount of UsualM burned.
     * @return unwrapped The amount of WrappedM tokens withdrawn.
     */
    function _unwrap(address account, address recipient, uint256 amount) internal returns (uint256 unwrapped) {
        _burn(account, amount);

        // NOTE: The behavior of `IWrappedMLike.transfer` is known, so its return can be ignored.
        IWrappedMLike(wrappedM()).transfer(recipient, unwrapped = amount);
    }

Here the return value of .transfer call was deliberately ignored assuming that the behavior of IWrappedMLike.transfer is known. But WrappedMToken is a Migratable contract and can be migrated to a new version in the future. The behavior of IWrappedMLike.transfer function of the future implementation contract is unknown. Instead of reverting, the .transfer call might return false upon failure which UsualM::_unwrap function won't be able to catch.

Internal Pre-conditions

1. The UsualM contract having zero WrappedM balance or a balance that is less than the amount of burned UsualM token.

External Pre-conditions

1. The WrappedMToken contract will have to migrate to a new version where instead of reverting, the .transfer call will return false upon failure.

Impact

Users will lose UsualM token but won't receive any WrappedM back.

PoC

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.26;

import { Test } from "../../../lib/forge-std/src/Test.sol";
import { MockRegistryAccess } from "../../utils/Mocks.sol";
import {
    DEFAULT_ADMIN_ROLE,
    USUAL_M_UNWRAP,
    USUAL_M_MINTCAP_ALLOCATOR
} from "../../../src/usual/constants.sol";
import { UsualM } from "../../../src/usual/UsualM.sol";

contract MockWrappedM_V2 {
    mapping(address account => uint256 balance) public balanceOf;
    function transfer(address recipient, uint256 amount) external returns (bool) {
        // return false instead of revert;
        if (balanceOf[msg.sender] < amount) return false;   

        balanceOf[msg.sender] -= amount;
        balanceOf[recipient] += amount;

        return true;
    }
    function transferFrom(address sender, address recipient, uint256 amount) external returns (bool) {
        // return false instead of revert;
        if (balanceOf[sender] < amount) return false;   

        balanceOf[sender] -= amount;
        balanceOf[recipient] += amount;

        return true;
    }
    function setBalanceOf(address account, uint256 balance) external {
        balanceOf[account] = balance;
    }
}

contract UsualMTests is Test {
    event MintCapSet(uint256 newMintCap);
    address internal _admin = makeAddr("admin");
    address internal _alice = makeAddr("alice");
    address internal _mintCapAllocator = makeAddr("mintCapAllocator");

    MockWrappedM_V2 internal _wrappedM;
    MockRegistryAccess internal _registryAccess;
    UsualM internal _usualM;
    function setUp() external {
        _wrappedM = new MockWrappedM_V2();
        _registryAccess = new MockRegistryAccess();

        // Set default admin role.
        _registryAccess.grantRole(DEFAULT_ADMIN_ROLE, _admin);

        _usualM = new UsualM();
        _resetInitializerImplementation(address(_usualM));
        _usualM.initialize(address(_wrappedM), address(_registryAccess));

        _registryAccess.grantRole(USUAL_M_UNWRAP, _alice);

        // Add mint cap allocator role to a separate address
        vm.prank(_admin);
        _registryAccess.grantRole(USUAL_M_MINTCAP_ALLOCATOR, _mintCapAllocator);

        // Set an initial mint cap
        vm.prank(_mintCapAllocator);
        _usualM.setMintCap(10_000e6);
    }
    
    /* ============ Users losing UsualM, not getting any WrappedM back ============ */
    function test_unwrap() external {
        vm.prank(_alice);
        _usualM.wrap(_alice, 5e6);

        vm.prank(_alice);
        _usualM.unwrap(_alice, 5e6);

        assertEq(_wrappedM.balanceOf(_alice), 0);   // alice didn't get any WrappedM back.
        assertEq(_usualM.balanceOf(_alice), 0);   // alice lost 5 UsualM.
    }

    /* ============ utils ============ */
    function _resetInitializerImplementation(address implementation) internal {
        // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Initializable")) - 1)) & ~bytes32(uint256(0xff))
        bytes32 INITIALIZABLE_STORAGE = 0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a00;
        // Set the storage slot to uninitialized
        vm.store(address(implementation), INITIALIZABLE_STORAGE, 0);
    }
}

Mitigation

Add a return value check after IWrappedMLike(wrappedM()).transfer call or use SafeERC20.safeTransfer.