# #45604 \[SC-Low] User Overpayment in \`transferToCoreVault\` Fee Handling **Submitted on May 17th 2025 at 19:31:37 UTC by @lufP for** [**Audit Comp | Flare | FAssets**](https://immunefi.com/audit-competition/audit-comp-flare-fassets) * **Report ID:** #45604 * **Report Type:** Smart Contract * **Report severity:** Low * **Target:** * **Impacts:** * Contract fails to deliver promised returns, but doesn't lose value ## Description **Affected Component(s):** * Contract: `CoreVault.sol` * Library: `Transfers.sol` (specifically the `TRANSFER_GAS_ALLOWANCE` constant) * Function: `transferToCoreVault(Agent.State storage _agent, uint64 _amountAMG)` \*\* Summary:\*\* The `transferToCoreVault` function in `CoreVault.sol` has a specific condition in its fee payment and refund logic that can lead to a user overpaying by exactly `Transfers.TRANSFER_GAS_ALLOWANCE` (100,000 Wei). This overpaid amount is sent to the fee collector (`state.nativeAddress`) instead of being refunded to the user. \*\* Detailed Description:\*\* The fee handling logic in `transferToCoreVault` is as follows: ```solidity // contracts/assetManager/library/CoreVault.sol // ... uint256 transferFeeWei = getTransferFee(transferredAMG); require(msg.value >= transferFeeWei, "transfer fee payment too small"); // User must send at least the fee // ... // pay the transfer fee and return overpaid transfer fee when the difference is larger than gas use if (msg.value > transferFeeWei + Transfers.TRANSFER_GAS_ALLOWANCE) { // Scenario 1: Significant overpayment Transfers.transferNAT(state.nativeAddress, transferFeeWei); // Fee collector gets the exact calculated fee Transfers.transferNATAllowFailure(payable(msg.sender), msg.value - transferFeeWei); // User gets the remainder refunded } else { // Scenario 2: msg.value is between transferFeeWei (inclusive) // and transferFeeWei + Transfers.TRANSFER_GAS_ALLOWANCE (inclusive) Transfers.transferNAT(state.nativeAddress, msg.value); // Fee collector gets the entire msg.value sent by the user } // ... ``` The constant `Transfers.TRANSFER_GAS_ALLOWANCE` is defined in `contracts/utils/lib/Transfers.sol`: ```solidity // contracts/utils/lib/Transfers.sol uint256 internal constant TRANSFER_GAS_ALLOWANCE = 100_000; // Value is 100,000 (Wei) ``` The issue arises when a user sends `msg.value` such that: `transferFeeWei < msg.value <= transferFeeWei + Transfers.TRANSFER_GAS_ALLOWANCE` Specifically, if `msg.value == transferFeeWei + Transfers.TRANSFER_GAS_ALLOWANCE` (i.e., `transferFeeWei + 100_000` Wei): 1. The condition `msg.value > transferFeeWei + Transfers.TRANSFER_GAS_ALLOWANCE` is **false**. 2. The `else` block is executed: `Transfers.transferNAT(state.nativeAddress, msg.value)`. 3. In this case, `state.nativeAddress` (the fee collector) receives the entire `msg.value`, which is `transferFeeWei + 100_000` Wei. 4. The user does not receive any refund. Effectively, they pay the intended `transferFeeWei` plus an additional `100_000` Wei, which is unnecessarily captured by the fee collector. \*\* Impact:\*\* * **Financial:** Minor financial loss for users who happen to send `msg.value` within the affected range. The maximum loss per transaction due to this specific issue is `Transfers.TRANSFER_GAS_ALLOWANCE` (100,000 Wei). * **User Experience:** Users might be confused about the exact fee deduction if they notice the discrepancy. * **Systemic Risk:** None. This does not compromise the overall security or fund safety of the asset management system beyond the minor overpayment. \*\* Code Snippet with Issue:\*\* ```solidity // contracts/assetManager/library/CoreVault.sol: Lines ~79-85 // ... if (msg.value > transferFeeWei + Transfers.TRANSFER_GAS_ALLOWANCE) { // TRANSFER_GAS_ALLOWANCE = 100_000 Transfers.transferNAT(state.nativeAddress, transferFeeWei); Transfers.transferNATAllowFailure(payable(msg.sender), msg.value - transferFeeWei); } else { // BUG: If msg.value is, for example, transferFeeWei + 100_000, // this branch is taken, and nativeAddress receives transferFeeWei + 100_000. // The user overpays by 100_000 Wei. Transfers.transferNAT(state.nativeAddress, msg.value); } // ... ``` \*\* Suggested Fix(es):\*\* To ensure the user only pays the `transferFeeWei` and any amount above it (that is not a very large overpayment handled by the `if` branch) is refunded, the `else` block could be modified. **Option A (Consistent Refund for Minor Overpayments):** Modify the `else` branch to also refund amounts greater than `transferFeeWei`. ```solidity // ... if (msg.value > transferFeeWei + Transfers.TRANSFER_GAS_ALLOWANCE) { Transfers.transferNAT(state.nativeAddress, transferFeeWei); Transfers.transferNATAllowFailure(payable(msg.sender), msg.value - transferFeeWei); } else { // This branch now handles msg.value >= transferFeeWei && msg.value <= transferFeeWei + TRANSFER_GAS_ALLOWANCE Transfers.transferNAT(state.nativeAddress, transferFeeWei); // Collector gets the exact fee if (msg.value > transferFeeWei) { Transfers.transferNATAllowFailure(payable(msg.sender), msg.value - transferFeeWei); // Refund the small overpayment } // If msg.value == transferFeeWei, no refund is needed. } // ... ``` This ensures that if `msg.value` is, for instance, `transferFeeWei + 50_000`, the user gets `50_000` Wei back. **Option B (Simplified Logic - User always pays at least `transferFeeWei`, excess always refunded if `TRANSFER_GAS_ALLOWANCE` is removed from condition):** If `TRANSFER_GAS_ALLOWANCE` is not meant to be a threshold for a different fee tier but rather a gas stipend for the refund transaction itself (which is how `Transfers.transferNATAllowFailure` uses it internally for its own gas), the logic could be simplified: The `Transfers.TRANSFER_GAS_ALLOWANCE` in the conditional `msg.value > transferFeeWei + Transfers.TRANSFER_GAS_ALLOWANCE` is somewhat misleading if its only purpose is to decide whether a refund *occurs at all* versus being a small additional fixed fee. If the goal is: "User pays `transferFeeWei`. If they send more, refund the difference, provided the difference is enough to be worth refunding (e.g., covers refund gas)", then the existing `if` captures this for *large* differences. The `else` currently captures *small* differences as an extra fee. If the intention is that any amount `> transferFeeWei` should be refunded, then: ```solidity // ... // Minimum payment is already ensured by: require(msg.value >= transferFeeWei, "transfer fee payment too small"); Transfers.transferNAT(state.nativeAddress, transferFeeWei); // Collector always gets the exact fee if (msg.value > transferFeeWei) { // Refund any amount paid over the exact transferFeeWei // The Transfers.transferNATAllowFailure function already has a gas stipend (TRANSFER_GAS_ALLOWANCE) for the .call Transfers.transferNATAllowFailure(payable(msg.sender), msg.value - transferFeeWei); } // ... ``` This simplifies the logic: the collector gets `transferFeeWei`, and any overpayment is refunded. The `Transfers.TRANSFER_GAS_ALLOWANCE` constant's role would then primarily be for the internal gas forwarding of the `transferNAT` functions, not for the conditional fee logic in `CoreVault`. \*\* Test Case Suggestion:\*\* A new test case should be added to specifically verify the behavior when `msg.value == transferFeeWei + Transfers.TRANSFER_GAS_ALLOWANCE`. ```typescript // In test/integration/fasset-simulation/14-CoreVault.ts it("should correctly handle fee when msg.value is exactly transferFeeWei + TRANSFER_GAS_ALLOWANCE", async () => { // ... (setup agent, minter, calculate transferAmount and cbTransferFee) ... const transferGasAllowance = toBN(100000); // Assuming Transfers.TRANSFER_GAS_ALLOWANCE is 100_000 Wei const criticalMsgValue = cbTransferFee.add(transferGasAllowance); const balanceBefore = await context.wNat.balanceOf(agent.ownerWorkAddress); // Or web3.eth.getBalance if it's plain NAT const res = await context.assetManager.transferToCoreVault(agent.vaultAddress, transferAmount, { from: agent.ownerWorkAddress, value: criticalMsgValue }); const balanceAfter = await context.wNat.balanceOf(agent.ownerWorkAddress); const txCost = await getTransactionCost(res); // Helper to get gas cost of the transaction itself const expectedBalanceAfter = balanceBefore.sub(cbTransferFee).sub(txCost); // If bug fixed (Option A/B) // Current (buggy) behavior check: // const expectedBalanceAfterBuggy = balanceBefore.sub(criticalMsgValue).sub(txCost); // assertWeb3Equal(balanceAfter, expectedBalanceAfterBuggy, "User's balance should decrease by criticalMsgValue + gas"); // To check if fixed (e.g., with Option A or B from suggestions): // The sender should have effectively paid only cbTransferFee + actual transaction gas. // Their balance should decrease by cbTransferFee + txCost. The 100,000 Wei should be refunded. assertWeb3Equal(balanceAfter, expectedBalanceAfter, "User should be refunded the TRANSFER_GAS_ALLOWANCE amount"); // Additionally, check the fee collector's balance. // It should only increase by cbTransferFee. }); ``` ## Proof of Concept ## Proof of Concept \*\* Steps to Reproduce:\*\* 1. Identify an agent and an amount (`_amountAMG`) for a `transferToCoreVault` operation. 2. Calculate the required `transferFeeWei` using `getTransferFee(_amountAMG)` (or `assetManager.transferToCoreVaultFee(amountUBA)` from client side). 3. The `Transfers.TRANSFER_GAS_ALLOWANCE` constant is `100_000` Wei. 4. Call `transferToCoreVault` with `msg.value` set to exactly `transferFeeWei + 100_000` Wei. 5. Observe: * The transaction succeeds. * The sender's native token balance will decrease by `transferFeeWei + 100_000` Wei. * The `state.nativeAddress` balance will increase by `transferFeeWei + 100_000` Wei. * The sender will not receive a refund for the `100_000` Wei. **4. Expected Behavior:** Ideally, the user should only pay the calculated `transferFeeWei`. Any amount sent above this, if not intended for another purpose (like covering a *variable* gas cost returned to the user, which `TRANSFER_GAS_ALLOWANCE` seems to hint at but isn't implemented as such a refund mechanism here), should be refunded. Alternatively, if `TRANSFER_GAS_ALLOWANCE` is an intentional, small, fixed additional fee for transactions that don't significantly overpay, this should be clearly documented and the logic possibly refactored for clarity (e.g., `effectiveFee = transferFeeWei + SMALL_FIXED_MARGIN_FEE`). Given the naming (`TRANSFER_GAS_ALLOWANCE`) and the `if` condition checking for "difference is larger than gas use", the current behavior where this amount is silently added to the collected fee in the `else` branch is unexpected and leads to overpayment. A more intuitive behavior for the `else` branch might be to transfer `transferFeeWei` to the collector and refund `msg.value - transferFeeWei` to the user if `msg.value > transferFeeWei`. \*\* Actual Behavior:\*\* If `msg.value` is between `transferFeeWei + 1` Wei and `transferFeeWei + 100,000` Wei (inclusive), the user pays the entire `msg.value` to the fee collector. The maximum overpayment in this specific path is `100,000` Wei. --- # Agent Instructions: Querying This Documentation If you need additional information that is not directly available in this page, you can query the documentation dynamically by asking a question. Perform an HTTP GET request on the current page URL with the `ask` query parameter: ``` GET https://reports.immunefi.com/flare-fassets-or-mainnet-audit-comp/45604-sc-low-user-overpayment-in-transfertocorevault-fee-handling.md?ask= ``` The question should be specific, self-contained, and written in natural language. The response will contain a direct answer to the question and relevant excerpts and sources from the documentation. Use this mechanism when the answer is not explicitly present in the current page, you need clarification or additional context, or you want to retrieve related documentation sections.