USDC is a well known token that has a fee switch that could be activated at any time.
If this happens, any PerpetualVault using it will be broken on deposits, as the amount
of token used to mint shares do not account for fees.
The issue will cause total deposited amount and users deposit amounts to be wrongly accounted:
Every time deposit
is called the totalDepositAmount
will be wrong
For users deposit, on first deposit, this one is the less inconvenient as its very unlikely to happen
on any deposits made in a vault with an open position, so on most of the deposits
I've listed some situations where this will cause wrong accounting, but there might be others.
During first deposit, we see L230
that the totalDepositedAmount
is incremented by amount
, and depositInfo[counter]
initialized with amount
too, while when FoT are in play, the real value received by the vault will be in reality amount - someFee
.
We also see L235
that the deposit
function will directly call the _mint
function (as positionIsClosed
is set to true on initialization), which will then mint to the user an amount of shares equal to depositInfo[depositId].amount * 1e8
where depositInfo[depositId].amount == amount
.
Once the vault has received deposits and opened a position, the deposit
function will not directly mint shares to the users, but rather set the nextAction.selector
to INCREASE_ACTION
.
This will trigger keepers into calling the runNextAction()
, which will then call _createIncreasePosition
in case of a vault with leverage >1, creating a GMX order.
Once the GMX order has been successfully created, the afterOrderExecution
will be called, and the FLOW.DEPOSIT
case will be executed.
We then see, L484
that the amount that will be used to mint shares to the users is read again from depositInfo[counter].amount
, which as we shown before that, do not account for FoT.
Finally, that amount is used to compute increased
at L490
(where feeAmount
are GMX fees), and fed to the _mint
function L494
.
Wrong accounting for USDC if fees are activated.
Likelihood: low, as this require USDC to activate fees and we don't know when this will happens
Impact: high, as this would break accounting for the vault
Rather than relying of the amount
input in deposit()
, measure the actual received amount by comparing balanceBefore
and balanceAfter
.
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."
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.
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."
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.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.