DeFiFoundry
50,000 USDC
View results
Submission Details
Severity: low
Invalid

Incorrect Collateral Token Accounting for Future Token Support

Summary

The deposit function assumes the full amount of collateralToken is transferred to the contract. While the currently supported tokens (WETH, WBTC, LINK, USDC) do not charge transfer fees, this logic becomes a vulnerability if the protocol ever supports fee-on-transfer tokens (e.g., USDT) in the future. The code does not account for discrepancies between the recorded deposit amount and the actual received amount, leading to systemic risks if token support expands.


Vulnerability Details

Affected Code

collateralToken.safeTransferFrom(msg.sender, address(this), amount);
depositInfo[counter].amount = amount; // Assumes full `amount` is received
totalDepositAmount += amount;

Root Cause

  • Assumption of Fee-Free Transfers: The code assumes all collateralToken transfers are fee-free. This is valid for the current tokens (WETH, WBTC, LINK, USDC) but not future-proof.

  • No Balance Checks: The contract does not verify the actual received token amount after transfers.

Current Token Behavior

Token Transfer Fee?
WETH No
WBTC No
LINK No
USDC No

Impact

Immediate Impact (Current Tokens)

  • None: Since WETH, WBTC, LINK, and USDC do not charge fees, the accounting remains accurate.

Future Impact (If Token Support Expands)

  1. Accounting Mismatch:

    • If a fee-charging token (e.g., USDT) is added as collateralToken, totalDepositAmount will exceed the actual contract balance.

    • Example: Depositing 1000 USDT (2% fee) would record +1000 in totalDepositAmount but only receive 980 USDT.

  2. Withdrawal Failures:

    • Users would be unable to withdraw their full deposits due to insufficient contract balances.

  3. Broken Share Calculations:

    • Shares minted based on inflated totalDepositAmount would dilute the value for existing users.


Proof of Concept (PoC)

Scenario: Future Support for USDT (2% Transfer Fee)

  1. Protocol Change:

    • Admin sets collateralToken = USDT (hypothetically).

  2. User Action:

    • User deposits 1000 USDT.

    • USDT contract deducts 2% (20 USDT) → Contract receives 980 USDT.

  3. Contract State:

    • depositInfo[counter].amount = 1000 USDT (incorrect).

    • totalDepositAmount += 1000 USDT (now 1000 USDT recorded vs. 980 USDT actual).

  4. Withdrawal Attempt:

    • User requests withdrawal of 1000 USDT.

    • Contract attempts to send 1000 USDT but only holds 980Transaction reverts.


Recommended Mitigation

Measure Actual Received Amount

Calculate the difference in the contract’s balance before and after the transfer, even for fee-free tokens:

uint256 balanceBefore = collateralToken.balanceOf(address(this));
collateralToken.safeTransferFrom(msg.sender, address(this), amount);
uint256 received = collateralToken.balanceOf(address(this)) - balanceBefore;
depositInfo[counter].amount = received;
totalDepositAmount += received;
Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.