Target
Description
Bug Description
In PerpV2Controller, positionInWantToken() view calculates value held in Perp account, which is updated by keeper on L1 using PerpTradeExecutor's setPosValue. Value is calculated using the following block:
int256 posValue = clearingHouse.getAccountValue(address(this));
uint256 amountOut = (posValue < 0)
? uint256(-1 * posValue)
: uint256(posValue);
The problem is confusion regarding if accountValue is negative. It does not mean value in short position, which is the case in accountBalance.getTotalPositionValue(), but actually that the account is underwater. Therefore, when the high-risk weekly Perp strategy goes negative, it will show up as positive funds! This has several severe consequences detailed under impacts section.
Impact
When batcher deposits user's funds, it will return a smaller amount of shares than should be, therefore users immediately lose value after negative position is settled.
When batcher withdraws user's funds, it will return too many tokens to the user, thinking there are more funds than in reality. Therefore, protocol solvency is at risk.
In fee collection, system will collect too much fees because totalExecutorFunds() is miscalculated.
If the system supports non-batch mode, users can directly exploit this by withdrawing their funds immediately.
Risk Breakdown
Difficulty to Exploit: Easy
Weakness: Sign confusion
Likelihood: High
Recommendation
In positionInWantTokens(), is posValue < 0, the holding is worthless, return 0.
References
Perpetual code samples (https://support.perp.com/hc/en-us/articles/7917807368729-Perp-v2-Integration-Guide-Code-Samples)
Perpetual source code (https://github.com/perpetual-protocol/perp-curie-contract)
Proof of Concept
Protocol insolvency scenario:
Protocol currently has 9,000 USDC value
User deposits 1,000 USDC, receives 1/10 total shares
Protocol has 200 USDC yield, invests it in perpetual
pertual investment backfires, positionvalue goes to -400
pool total funds is calculated as 9,000 + 1000 + 400 = 10,400
actual total funds: 9,000 + 1000 = 10,000
User withdraws from pool, receives 1/10 of total value = 1,040 (minus fees)
Therefore, user pulled out of vault +40 more than share is worth.
Brahma.Fi closed this submission saying that accountValue can never be negative as the position will be liquidated at margin value of 6.25%. This statement is ridiculous, since they are counting on external liquidator bots to decide it's time to liquidate, in order for the protocol to remain solvent. The sign confusion is clearly unintentional and dangerous, but they had to come up with some excuse not to mark the report as valid.
Comments