Potential Discrepancy Between Specified and Actual Token Transfer Amounts

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x1c59c2346e8d70837bd9c3a0ae00359496f372f23e0c779095011d6aabff5766
Severity: medium

Description:
Description

The ERC20Minter, NativeMinterRedeem, and BaseMinterWithdrawal contracts contain functions (deposit, redeem, and requestWithdrawal) that interact with arbitrary ERC20 tokens. These functions assume that the exact amount specified in the function call will be transferred. However, some tokens (like cUSDCv3) have non-standard transfer implementations that may not transfer the exact amount specified, leading to potential discrepancies between the amount the contract thinks it received and the amount it actually received.

This vulnerability affects multiple functions across different contracts in the system, potentially impacting the integrity of deposits, redemptions, and withdrawal requests.

Attack Scenario

An attacker could exploit this vulnerability using a token with a non-standard transfer implementation, such as cUSDCv3. A potential scenario could be:

1. The attacker has a small balance (e.g., 100 units) of a non-standard token (like cUSDCv3).
2. The attacker calls the deposit() function with an amount of type(uint256).max.
3. The non-standard token's transfer function interprets type(uint256).max as a signal to transfer the entire balance.
4. Only 100 units are actually transferred to the contract.
5. However, the contract proceeds to mint staking tokens based on the original type(uint256).max amount.
6. This results in the attacker receiving far more staking tokens than they should, based on their actual deposit.

Similar scenarios could play out with the redeem() and requestWithdrawal() functions, potentially allowing an attacker to redeem or withdraw more tokens than they should be able to.

Attachments

1. Proof of Concept

// ERC20Minter contract
function deposit(uint256 amount, address receiver) public virtual nonReentrant {
    require(amount > 0, "ZeroDeposit");
    uint256 mintAmount = previewDeposit(amount);
    require(mintAmount > 0, "ZeroMintAmount");
    baseToken.safeTransferFrom(address(msg.sender), address(this), amount);
    stakingToken.mint(receiver, mintAmount);
    emit Deposit(address(msg.sender), receiver, amount);
}

// NativeMinterRedeem contract
function redeem(uint256 amount, address receiver) public virtual nonReentrant {
    require(amount > 0, "ZeroRedeem");
    uint256 redeemAmount = previewRedeem(amount);
    require(redeemAmount > 0, "ZeroRedeemAmount");
    stakingToken.safeTransferFrom(address(msg.sender), address(this), amount);
    stakingToken.burn(amount);
    SafeTransferLib.safeTransferETH(receiver, redeemAmount);
    emit Redeem(address(msg.sender), receiver, amount);
}

// BaseMinterWithdrawal contract
function requestWithdrawal(uint256 amount, address receiver) public nonReentrant {
    require(amount >= minWithdrawal, "LessThanMin");
    uint256 netAmount = previewWithdrawal(amount);
    stakingToken.safeTransferFrom(msg.sender, address(this), amount);

    uint256 withdrawalId = nextWithdrawalId++;
    _safeMint(receiver, withdrawalId);

    _withdrawalRequests[withdrawalId] = WithdrawalRequest({
        amount: netAmount,
        processed: false,
        claimed: false
    });

    totalPendingWithdrawals = totalPendingWithdrawals+netAmount;
    totalWithdrawalFees = totalWithdrawalFees+amount-netAmount;

    emit RequestWithdrawal(address(msg.sender), receiver, amount, withdrawalId);
}

Revised Code Suggestion

// ERC20Minter contract
function deposit(uint256 amount, address receiver) public virtual nonReentrant {
    require(amount > 0, "ZeroDeposit");
+   require(amount < type(uint256).max, "AmountTooLarge");
    uint256 mintAmount = previewDeposit(amount);
    require(mintAmount > 0, "ZeroMintAmount");
+   uint256 balanceBefore = baseToken.balanceOf(address(this));
    baseToken.safeTransferFrom(address(msg.sender), address(this), amount);
+   uint256 actualTransferred = baseToken.balanceOf(address(this)) - balanceBefore;
+   require(actualTransferred == amount, "TransferAmountMismatch");
-   stakingToken.mint(receiver, mintAmount);
+   stakingToken.mint(receiver, previewDeposit(actualTransferred));
-   emit Deposit(address(msg.sender), receiver, amount);
+   emit Deposit(address(msg.sender), receiver, actualTransferred);
}

// Similar changes should be applied to the redeem() function in NativeMinterRedeem
// and the requestWithdrawal() function in BaseMinterWithdrawal

These revisions add checks to ensure the actual transferred amount matches the specified amount, and use the actual transferred amount for subsequent operations. This protects against discrepancies that could arise from non-standard token implementations.

The fix addresses the vulnerability by:

1. Adding an upper bound check to prevent potential overflow issues with extremely large inputs.
2. Verifying the actual transferred amount against the expected amount.
3. Using the actual transferred amount for minting, burning, or recording withdrawal requests, and in event emissions.

[0xRizwan]

Please check this comment by sponsor.

Only Native chain tokens are expected to be used so its not intended to use tokens like cUSDCv3. Its non-issue.