Unsafe use of transferFrom() in Migration Contract

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x25a3341ce9c3658eb0cc5154469dbe196af078867c309a74bc35a888a082afe2 Severity: low

Description:

Description

The Migration contract, responsible for migrating tokens from Circles v1 to v2, contains a potential vulnerability due to the unsafe use of the transferFrom() function. This issue is present in the migrate() function, which is central to the token migration process.

The vulnerability stems from the lack of proper error handling when calling transferFrom(). The function does not check the return value of the transfer operation, nor does it use a safe transfer method. This can lead to silent failures during the migration process, potentially resulting in a mismatch between v1 tokens transferred and v2 tokens minted.

Location: Migration contract, migrate() function

Potential Consequences

If SafeTransfer is not implemented, several issues could arise:

1. Silent Failures:

   • Transfer operations could fail without the contract being aware, leading to inconsistent state.
   • Users might believe their tokens have been migrated when, in fact, the transfer failed.

2. Token Loss:

   • In case of a failed transfer, v2 tokens might be minted without the corresponding v1 tokens being received.
   • This could lead to inflation in the v2 token supply without proper backing from v1 tokens.

3. Inconsistent Contract State:

   • The contract's internal accounting of migrated tokens could become inaccurate.
   • This could lead to discrepancies between the actual token balances and what the contract believes to be true.

4. Vulnerability to Non-Standard ERC20 Implementations:

   • Some ERC20 tokens don't return a boolean for transfer and transferFrom.
   • The current implementation wouldn't catch failures with these non-standard tokens.

1. Proof of Concept

    function migrate(address[] calldata _avatars, uint256[] calldata _amounts) external returns (uint256[] memory) {
        if (_avatars.length != _amounts.length) {
            // Arrays length mismatch.
            revert CirclesArraysLengthMismatch(_avatars.length, _amounts.length, 0);
        }

        uint256[] memory convertedAmounts = new uint256[](_avatars.length);

        for (uint256 i = 0; i < _avatars.length; i++) {
            ITokenV1 circlesV1 = ITokenV1(hubV1.userToToken(_avatars[i]));
            if (address(circlesV1) == address(0)) {
                // Invalid avatar, not registered in hub V1.
                revert CirclesAddressCannotBeZero(2);
            }
            convertedAmounts[i] = convertFromV1ToDemurrage(_amounts[i]);
            // Vulnerable line: transfer the v1 Circles to this contract to be locked
            circlesV1.transferFrom(msg.sender, address(this), _amounts[i]);
        }

        // mint the converted amount of v2 Circles
        hubV2.migrate(msg.sender, _avatars, convertedAmounts);

        return convertedAmounts;
    }

    // ... (rest of the contract code)
}

This PoC showcases the existing vulnerable code in the Migration contract. The issue is in the migrate() function, specifically the line:

circlesV1.transferFrom(msg.sender, address(this), _amounts[i]);

This line uses the unsafe transferFrom() method without checking its return value or using a safe transfer method, which can lead to silent failures and the potential consequences described above.

2. Revised Code File

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity >=0.8.24;

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "../errors/Errors.sol";
import "../hub/IHub.sol";
import "./IHub.sol";
import "./IToken.sol";

contract Migration is ICirclesErrors {
    using SafeERC20 for IERC20;

    // ... (rest of the contract remains the same)

    function migrate(address[] calldata _avatars, uint256[] calldata _amounts) external returns (uint256[] memory) {
        if (_avatars.length != _amounts.length) {
            revert CirclesArraysLengthMismatch(_avatars.length, _amounts.length, 0);
        }

        uint256[] memory convertedAmounts = new uint256[](_avatars.length);

        for (uint256 i = 0; i < _avatars.length; i++) {
            ITokenV1 circlesV1 = ITokenV1(hubV1.userToToken(_avatars[i]));
            if (address(circlesV1) == address(0)) {
                revert CirclesAddressCannotBeZero(2);
            }
            convertedAmounts[i] = convertFromV1ToDemurrage(_amounts[i]);

            // Use SafeERC20's safeTransferFrom
            IERC20(address(circlesV1)).safeTransferFrom(msg.sender, address(this), _amounts[i]);
        }

        // mint the converted amount of v2 Circles
        hubV2.migrate(msg.sender, _avatars, convertedAmounts);

        return convertedAmounts;
    }

    // ... (rest of the contract remains the same)
}

Explanation of the fix:

1. Import OpenZeppelin's SafeERC20 library at the top of the contract.
2. Sdd the using SafeERC20 for IERC20; statement to enable the use of safe transfer functions.
3. In the migrate() function, we replace the unsafe transferFrom() call with safeTransferFrom():

   IERC20(address(circlesV1)).safeTransferFrom(msg.sender, address(this), _amounts[i]);

This change ensures that:

• The transfer will revert if it fails, preventing silent failures.
• It's compatible with both standard and non-standard ERC20 implementations.
• It provides clear error messages if the transfer fails.

By implementing this fix, we improve the security and reliability of the migration process. Any transfer failure will now cause the entire transaction to revert, preventing inconsistencies between v1 token transfers and v2 token minting.

[benjaminbollen]

Thank you for your report on the potential unsafe use of transferFrom() in the Migration Contract. After careful review, we've determined that this is not an issue.

The transferFrom() function is called on CirclesV1 tokens, which correctly implement the ERC20 standard. This implementation ensures that the function behaves as expected and does not cause any problems within our system.

We appreciate your attention to potential security vulnerabilities in our contract interactions. Your diligence in examining our code contributes to the overall robustness of our platform. Thank you for your efforts in helping ensure the security of our system.

[Naties29]

@benjaminbollen I would like to escalate this issue, again as I feel it is perfectly valid I believe this issue deserves reconsideration for the following reasons.

Not all ERC20 tokens follow the standard correctly, and some (like USDT) don't return booleans for transfers.

The current implementation could lead to silent failures and state inconsistencies if a transfer fails.

Using OpenZeppelinn provides SafeERC20 specifically to address these concerns.

Implementing the fix is straightforward and significantly improves the contract's robustness with minimal downsides.

[kakarottosama]

@Naties29 it's clearly using ONLY CircleV1 token, not any other ERC20 or USDT

[Naties29]

@kakarottosama Thank you for clarifying!