diff --git a/README.md b/README.md
index fad0abf..052b869 100644
--- a/README.md
+++ b/README.md
@@ -1,469 +1,442 @@
-# Issue M-1: Traders may decrease their trading loss via mint/burn
+# Issue H-1: Fee Precision Loss Disrupts Liquidations and Causes Loss of Funds
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/12
+Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/66
## Found by
-0x37
+0xbranded
## Summary
-When traders have some trading loss, they can decrease their trading loss via mint LP before closing position and burn LP after closing position.
+Borrowing and funding fees of both longs/shorts suffer from two distinct sources of precision loss. The level of precision loss is large enough to consistently occur at a significant level, and can even result in total omission of fee payment for periods of time. This error is especially disruptive given the sensitive nature of funding fee calculations both in determining liquidations (a core functionality), as well as payments received by LPs and funding recipients (representing a significant loss).
## Vulnerability Detail
-When traders open one position with long or short size, traders have some negative unrealized Pnl. When traders close their positions, LP holders as traders' counterparty, will take traders' loss as their profits.
-The problem is that there is not any limitation for mint()/burn() functions. Traders can make use of this problem to decrease their trading loss.
-For example:
-1. Alice opens one Long position in BTC/USDT market.
-2. BTC price decreases and Alice's long position has some negative Pnl.
-3. Alice wants to close her position.
-4. Alice borrows one huge amount of BTC/USDT and mint in this BTC/USDT market to own lots of shares.
-5. Alice closes her position and her loss will become LP's profit, this will increase LP share's price.
-6. Alice burns her share with one higher share price.
-
-In this way, Alice can avoid most of her trading loss.
-
-## Impact
-Traders have ways to avoid trading loss. This means that LPs will lose some profits that they deserve.
-
-## Code Snippet
-https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/core.vy#L154-L226
-
-## Tool used
-Manual Review
-
-## Recommendation
-Add one LP locking mechanism. When someone mints some shares in the market, he cannot burn his shares immediately. After a cool down period, he can burn his shares.
-
-# Issue M-2: Incorrect borrowing fee calculation
+The first of the aforementioned sources of precision loss is relating to the `DENOM` parameter defined and used in `apply` of [`math.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/math.vy#L163-L172):
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/14
+```vyper
+DENOM: constant(uint256) = 1_000_000_000
-## Found by
-0x37, KupiaSec
-## Summary
-Traders with high collateral, low leverage will pay more borrowing fee than traders with low collateral, high leverage.
+def apply(x: uint256, numerator: uint256) -> Fee:
+ fee : uint256 = (x * numerator) / DENOM
+...
+ return Fee({x: x, fee: fee_, remaining: remaining})
+```
-## Vulnerability Detail
-Traders in markets can add some leverage in order to get higher profits. LPs have to lock `collateral * leverage` value token to match this leverage position. Traders have to pay some borrowing fee for the locked token. The borrowing fee should be related with locked token amount.
-The problem is that the borrowing fee is related with this position's collateral amount, not related with leverage. This could cause some positions with less locked token amount need to pay more borrowing fees.
-For example:
-- Alice opens one long position with 50000 USDT, 2x leverage in BTC/USDT market.
-- Bob opens one long position with 10000 USDT, 20x leverage in BTC/USDT market.
-- Lps have to lock more BTC tokens for Bob compared with Alice's position. However, Alice has to pay more borrowing fee because she has one higher collateral amount.
+This function is in turn referenced (to extract the `fee` parameter in particular) in several locations throughout [`fees.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/fees.vy#L265), namely in determining the funding and borrowing payments made by positions open for a duration of time:
```vyper
-@external
-@view
-def calc_fees(id: uint256) -> FeesPaid:
- pos : PositionState = Positions(self).lookup(id)
- pool : PoolState = self.POOLS.lookup(pos.pool)
- # Here collateral is this position's collateral.
- fees : SumFees = self.FEES.calc(
-@> pos.pool, pos.long, pos.collateral, pos.opened_at)
-```
-```vyper
-@external
-@view
-def calc(id: uint256, long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
- period: Period = self.query(id, opened_at)
+ paid_long_term : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
+...
+ paid_short_term : uint256 = self.apply(fs.short_collateral, fs.funding_short * new_terms)
+...
P_b : uint256 = self.apply(collateral, period.borrowing_long) if long else (
self.apply(collateral, period.borrowing_short) )
+ P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
+ self.apply(collateral, period.funding_short) )
```
-## Impact
-Trading positions with less locked interest may have to pay more borrowing fee.
-
-## Code Snippet
-https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/fees.vy#L263-L274
-
-## Tool used
-Manual Review
-
-## Recommendation
-Borrowing fee should be in propotion to positions' locked interest.
-
-# Issue M-3: Last LP in each pool may lose a few amount of reserve.
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/15
-
-## Found by
-0x37
-## Summary
-Last Lp's burn() may be reverted if malicious users leave 1 wei share in this pool.
-
-## Vulnerability Detail
-At the end of each core function, we will trigger Fee's update. In the fee's update, we need to calculate the Lp tokens' utilization. The problem is that the utilization calculation will be reverted if reserves is not zero but less than 100.
+The comments for `DENOM` specify that it's a "magic number which depends on the smallest fee one wants to support and the blocktime." In fact, given its current value of $10^{-9}$, the smallest representable fee per block is $10^{-7}$%. Given the average blocktimes of 2.0 sec on the [BOB chain](https://explorer.gobob.xyz/), there are 15_778_800 blocks in a standard calendar year. Combined with the fee per block, this translates to an annual fee of 1.578%.
-This scenario is possible, especially when malicious users intent to do this.
-- Malicious users can mint one very small share in one market, for example, 1 share.
-- When last LP wants to burn his shares, the left reserve will be one very small amount but larger than 0. Because malicious users hold 1 share.
-- When we want to update the fees, the transaction will be reverted because the utilization calculation will be reverted.
+However, this is also the interval size for annualized fees under the current system. As a result, any fee falling below the next interval will be rounded down. For example, given an annualized funding rate in the neighborhood of 15%, there is potential for a nearly 10% error in the interest rate if rounding occurs just before the next interval. This error is magnified the smaller the funding rates become. An annual fee of 3.1% would round down to 1.578%, representing an error of nearly 50%. And any annualized fees below 1.578% will not be recorded, representing a 100% error.
-The last LP holder can still burn his most share via leaving some amount of share(reserves) into the pool.
-Although this amount is not large, considering that this case could happen in every market, more LPs may take some loss because of this finding.
+The second source of precision loss combines with the aforementioned error, to both increase the severity and frequency of error. It's related to how percentages are handled in `params.vy`, particularly when the long/short utilization is calculated to determine funding & borrow rates. The [utilization](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L59-L63) is shown below:
```vyper
-@internal
-@pure
def utilization(reserves: uint256, interest: uint256) -> uint256:
- # Then the last LP want to withdraw, will be reverted.
-@> return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
+ return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
```
-```vyper
-@external
-@view
-def calc_mint(
- id : uint256,
- base_amt : uint256,
- quote_amt : uint256,
- total_supply: uint256,
- ctx : Ctx) -> uint256:
- pv: uint256 = self.MATH.value(Pools(self).total_reserves(id), ctx).total_as_quote
- mv: uint256 = self.MATH.value(Tokens({base: base_amt, quote: quote_amt}), ctx).total_as_quote
- return Pools(self).f(mv, pv, total_supply)
-
-# ts --> total supply
-@external
-@pure
-def f(mv: uint256, pv: uint256, ts: uint256) -> uint256:
- if ts == 0: return mv
- else : return (mv * ts) / pv
+This function is in turn used to calculate borrowing (and funding rates, following a slightly different approach that similarly combines the use of `utilization` and `scale`), in `[dynamic_fees](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L33-L55)` of `params.vy`:
+```vyper
+def dynamic_fees(pool: PoolState) -> DynFees:
+ long_utilization : uint256 = self.utilization(pool.base_reserves, pool.base_interest)
+ short_utilization: uint256 = self.utilization(pool.quote_reserves, pool.quote_interest)
+ borrowing_long : uint256 = self.check_fee(
+ self.scale(self.PARAMS.MAX_FEE, long_utilization))
+ borrowing_short : uint256 = self.check_fee(
+ self.scale(self.PARAMS.MAX_FEE, short_utilization))
+...
+def scale(fee: uint256, utilization: uint256) -> uint256:
+ return (fee * utilization) / 100
```
-## Impact
-Last LP holder in each pool may have to leave a few amount of reserve in this market.
-
-## Code Snippet
-https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L57-L63
-## Tool used
+Note that `interest` and `reserves` maintain the same precision. Therefore, the output of `utilization` will have just 2 digits of precision, resulting from the division of `reserves` by `100`. However, this approach can similarly lead to fee rates losing a full percentage point in their absolute value. Since the utilization is used by `dynamic_fees` to calculate the funding / borrow rates, when combined with the formerly described source of precision loss the error is greatly amplified.
-Manual Review
+Consider a scenario when the long open interest is 199_999 * $10^{18}$ and the reserves are 10_000_000 * $10^{18}$. Under the current `utilization` functionality, the result would be a 1.9999% utilization rounded down to 1%. Further assuming the value of `max_fee = 65` (this represents a 100% annual rate and 0.19% 8-hour rate), the long borrow rate would round down to 0%. Had the 1.9999% utilization rate not been rounded down 1%, the result would have been `r = 1.3`. In turn, the precision loss in `DENOM` would have effectively rounded down to `r = 1`, resulting in a 2.051% borrow rate rounded down to 1.578%.
-## Recommendation
-Make sure the utilization calculation will not be reverted.
+In other words, the precision loss in `DENOM` alone would have resulted in a 23% error in this case. But when combined with the precision loss in percentage points represented in `utilization`, a 100% error resulted. While the utilization and resulting interest rates will typically not be low enough to produce such drastic errors, this hopefully illustrates the synergistic combined impact of both sources of precision loss. Even at higher, more representative values for these rates (such as `r = 10`), errors in fee calculations exceeding 10% will consistently occur.
-# Issue M-4: Healthy positions may be liquidated because of inappropriate judgment
+## Impact
+All fees in the system will consistently be underpaid by a significant margin, across all pools and positions. Additionally trust/confidence in the system will be eroded as fee application will be unpredictable, with sharp discontinuities in rates even given moderate changes in pool utilization. Finally, positions will be subsidized at the expense of LPs, since the underpayment of fees will make liquidations less likely and take longer to occur. As a result, LPs and funding recipients will have lesser incentive to provide liquidity, as they are consistently being underpaid while taking on a greater counterparty risk.
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/17
+As an example, consider the scenario where the long open interest is 1_099_999 * $10^{18}$ and the reserves are 10_000_000 * $10^{18}$. Under the current `utilization` functionality, the result would be a 10.9999% utilization rounded down to 10%. Assuming `max_fee = 65` (100% annually, 0.19% 8-hour), the long borrow rate would be `r = 6.5` rounded down to `r = 6`. A 9.5% annual rate results, whereas the accurate result if neither precision loss occurred is `r = 7.15` or 11.3% annually. The resulting error in the borrow rate is 16%.
-## Found by
-0x0x0xw3, 0x37, 0xaliyah, 0xnbvc, bareli, oxwhite
-## Summary
-Healthy positions may be liquidated in one special case
+Assuming a long collateral of 100_000 * $10^{18}$, LPs would earn 9_500 * $10^{18}$, when they should earn 11_300 * $10^{18}$, a shortfall of 1_800 * $10^{18}$ from longs alone. Additional borrow fee shortfalls would occur for shorts, in addition to shortfalls in funding payments received.
-## Vulnerability Detail
-In `is_liquidatable()`, there are some comments about the position's health. `A position becomes liquidatable when its current value is less than
- a configurable fraction of the initial collateral`.
-The problem is that when `pnl.remaining` equals `required`, this position should be healthy and cannot be liquidated. But this edge case will be though as unhealthy and can be liquidated.
+Liquidation from borrow rates along should have taken 106 months based on the expected result of 11.3% per annum. However, under the 9.5% annual rate it would take 127 months to liquidate a position. This represents a 20% delay in liquidation time from borrow rates alone, not including the further delay caused by potential underpaid funding rates.
-```vyper
-@external
-@view
-def is_liquidatable(position: PositionState, pnl: PnL) -> bool:
- """
- A position becomes liquidatable when its current value is less than
- a configurable fraction of the initial collateral, scaled by
- leverage.
- """
- percent : uint256 = self.PARAMS.LIQUIDATION_THRESHOLD * position.leverage
- required: uint256 = (position.collateral * percent) / 100
- return not (pnl.remaining > required)
-```
+When PnL is further taken into account, these delays could mark the difference between a period of volatility wiping out a position. As a result, these losing positions could run for far longer than should otherwise occur and could even turn into winners. Not only are LP funds locked for longer as a result, they are at a greater risk of losing capital to their counterparty. On top of this, they are also not paid their rightful share of profits, losing out on funds to take on an unfavorably elevated risk.
-## Impact
-Healthy positions may be liquidated and take some loss.
+Thus, not only do consistent, material losses (significant fee underpayment) occur but a critical, core functionality malfunctions (liquidations are delayed).
## Code Snippet
-https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L117-L138
+In the included PoC, three distinct tests demonstrate the individual sources of precision loss, as well as their combined effect. Similar scenarios were demonstrated as discussed above, for example interest = 199_999 * $10^{18} with reserves = 10_000_000 * $10^{18}$ with a max fee of 65.
-## Tool used
+The smart contracts were stripped to isolate the relevant logic, and [foundry](https://github.com/0xKitsune/Foundry-Vyper) was used for testing. To run the test, clone the repo and place `Denom.vy` in vyper_contracts, and place `Denom.t.sol`, `Cheat.sol`, and `IDenom.sol` under src/test.
-Manual Review
+
+Denom.vy
-## Recommendation
-```diff
-- return not (pnl.remaining > required)
-+ return not (pnl.remaining >= required)
-```
+```vyper
-# Issue M-5: Penalized funding received token will be locked in the contract
+struct DynFees:
+ funding_long : uint256
+ funding_short : uint256
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/18
+struct PoolState:
+ base_collateral : uint256
+ quote_collateral : uint256
-## Found by
-0x37
-## Summary
-If one position's funding received is penalized, these funding received will be locked in the contract.
+struct FeeState:
+ t1 : uint256
+ funding_long : uint256
+ funding_short : uint256
+ long_collateral : uint256
+ short_collateral : uint256
+ funding_long_sum : uint256
+ funding_short_sum : uint256
+ received_long_sum : uint256
+ received_short_sum : uint256
-## Vulnerability Detail
-When we close one position, we will check the remaining collateral after we deduct the borrowing fee and possible funding paid. If there is not left collateral, the funding received will be set to 0 as one penalty.
-In this case, the trader will not receive the funding received. And the `funding_received_want` will not be deducted from base_collateral considering one long position.
-The problem is that traders with short position will still pay funding fees. This will cause `funding_received_want` will be left and anyone cannot withdraw this part.
+struct SumFees:
+ funding_paid : uint256
+ funding_received: uint256
-```vyper
-@external
-@view
-def calc_fees(id: uint256) -> FeesPaid:
- pos : PositionState = Positions(self).lookup(id)
- pool : PoolState = self.POOLS.lookup(pos.pool)
- # Here collateral is this position's collateral.
- fees : SumFees = self.FEES.calc(
- pos.pool, pos.long, pos.collateral, pos.opened_at)
- # @audit-fp funding_paid, borrowing_paid will be paid via collateral.
- c0 : uint256 = pos.collateral
- # Funding paid and borrowing paid will be paid via the collateral.
- c1 : Val = self.deduct(c0, fees.funding_paid)
- c2 : Val = self.deduct(c1.remaining, fees.borrowing_paid)
- # Funding fees prioritized over borrowing fees.
- # deduct funding fee at first, and then deduct borrowing fee.
- funding_paid : uint256 = c1.deducted
- borrowing_paid : uint256 = c2.deducted
- # collateral - funding paid fee - borrowing fee
- remaining : uint256 = c2.remaining
- funding_received: uint256 = 0 if remaining == 0 else (
- min(fees.funding_received, avail) )
-```
-```vyper
+struct Period:
+ funding_long : uint256
+ funding_short : uint256
+ received_long : uint256
+ received_short : uint256
+
+#starting point hardcoded
@external
-@view
-def value(id: uint256, ctx: Ctx) -> PositionValue:
- # Get position state.
- pos : PositionState = Positions(self).lookup(id)
- # All positions will eventually become liquidatable due to fees.
- fees : FeesPaid = Positions(self).calc_fees(id)
- pnl : PnL = Positions(self).calc_pnl(id, ctx, fees.remaining)
-
- deltas: Deltas = Deltas({
- base_interest : [self.MATH.MINUS(pos.interest)], # unlock LP tokens.
- quote_interest : [],
- base_transfer : [self.MATH.PLUS(pnl.payout),
- self.MATH.PLUS(fees.funding_received)],
- base_reserves : [self.MATH.MINUS(pnl.payout)], # base reserve: when traders win, they will get LP's base token,
- # funding fee is not related with LP holders.
-
-@> base_collateral : [self.MATH.MINUS(fees.funding_received)], # ->
- ...
- }) if pos.long else ...
+def __init__():
+ self.FEE_STORE = FeeState({
+ t1 : block.number,
+ funding_long : 1,
+ funding_short : 0,
+ long_collateral : 10_000_000_000_000_000_000_000_000,
+ short_collateral : 10_000_000_000_000_000_000_000_000,
+ funding_long_sum : 0,
+ funding_short_sum : 0,
+ received_long_sum : 0,
+ received_short_sum : 0,
+ })
-```
+ self.FEE_STORE_AT[block.number] = self.FEE_STORE
-## Impact
-Penalized funding received token will be locked in the contract
+ self.FEE_STORE2 = FeeState({
+ t1 : block.number,
+ funding_long : 1_999,
+ funding_short : 0,
+ long_collateral : 10_000_000_000_000_000_000_000_000,
+ short_collateral : 10_000_000_000_000_000_000_000_000,
+ funding_long_sum : 0,
+ funding_short_sum : 0,
+ received_long_sum : 0,
+ received_short_sum : 0,
+ })
+ self.FEE_STORE_AT2[block.number] = self.FEE_STORE2
-## Code Snippet
-https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L242-L272
+# hardcoded funding rates for the scenario where funding is positive
+@internal
+@view
+def dynamic_fees() -> DynFees:
+ return DynFees({
+ funding_long : 10,
+ funding_short : 0,
+ })
-## Tool used
+# #hardcoded pool to have 1e24 of quote and base collateral
+@internal
+@view
+def lookup() -> PoolState:
+ return PoolState({
+ base_collateral : 10_000_000_000_000_000_000_000_000,
+ quote_collateral : 10_000_000_000_000_000_000_000_000,
+ })
-Manual Review
-## Recommendation
-Once the funding received is penalized, we can consider to transfer this part funds to collector directly.
+FEE_STORE : FeeState
+FEE_STORE_AT : HashMap[uint256, FeeState]
-# Issue M-6: Hardcoded Value Breaks Core Contract Functionality
+FEE_STORE2 : FeeState
+FEE_STORE_AT2 : HashMap[uint256, FeeState]
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/27
+@internal
+@view
+def lookupFees() -> FeeState:
+ return self.FEE_STORE
-## Found by
-0xbranded, ydlee
-## Summary
-The use of a hardcoded value for the pool id in [`apy.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/api.vy) proxied calls makes every other pool in the system inaccessible.
+@internal
+@view
+def lookupFees2() -> FeeState:
+ return self.FEE_STORE2
-## Vulnerability Detail
-The comments indicate that `apy.vy` is the entry-point contract, which proxy's calls to core. However, each call made to `core.vy` includes a hardcoded value of 1 for the pool id:
-```vyper
- return self.CORE.mint(1, base_token, quote_token, lp_token, base_amt, quote_amt, ctx)
- return self.CORE.burn(1, base_token, quote_token, lp_token, lp_amt, ctx)
- return self.CORE.open(1, base_token, quote_token, long, collateral0, leverage, ctx)
- return self.CORE.close(1, base_token, quote_token, position_id, ctx)
- return self.CORE.liquidate(1, base_token, quote_token, position_id, ctx)
-```
+@internal
+@view
+def fees_at_block(height: uint256) -> FeeState:
+ return self.FEE_STORE_AT[height]
-Within each of these functions in [`core.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/core.vy#L168-L172), the following checks are enforced:
-```vyper
- pool : PoolState = self.POOLS.lookup(id)
-...
- assert pool.base_token == base_token , ERR_PRECONDITIONS
- assert pool.quote_token == quote_token, ERR_PRECONDITIONS
-```
+@internal
+@view
+def fees_at_block2(height: uint256) -> FeeState:
+ return self.FEE_STORE_AT2[height]
-However the pools contract is [designed to support](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L96) multiple pools with differing pool id:
-```vyper
-def fresh(
- symbol : String[65],
- base_token : address,
- quote_token: address,
- lp_token : address) -> PoolState:
- self._INTERNAL()
- return self.insert(PoolState({
- id : self.next_pool_id(),
-...
+@external
+def update():
+ fs: FeeState = self.current_fees()
+ fs2: FeeState = self.current_fees2()
-def next_pool_id() -> uint256:
- id : uint256 = self.POOL_ID
- nxt: uint256 = id + 1
- self.POOL_ID = nxt
- return nxt
-```
+ self.FEE_STORE_AT[block.number] = fs
+ self.FEE_STORE = fs
-Thus, any user action with `core.vy` can interact only with this first pool and all pools with an id differing from 1 will be inaccessible.
+ self.FEE_STORE_AT2[block.number] = fs2
+ self.FEE_STORE2 = fs2
-## Impact
-Core contract functionality is broken, leading to a DoS (which doesn't compromise funds, since pools cannot be interacted with to begin with).
+#math
+ZEROS: constant(uint256) = 1000000000000000000000000000
+DENOM: constant(uint256) = 1_000_000_000
+DENOM2: constant(uint256) = 1_000_000_000_000
-## Code Snippet
+@internal
+@pure
+def extend(X: uint256, x_m: uint256, m: uint256) -> uint256:
+ return X + (m*x_m)
-## Tool used
-
-Manual Review
-
-## Recommendation
-Add an `id: uint256` parameter to the mentioned functions of `api.vy`, and forward this value to the calls of `core.vy`, rather than hardcoding 1.
-
-# Issue M-7: DoS of LP Withdrawals Due to Abuse of `unlocked_reserves`
-
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/28
-
-## Found by
-0xbranded
-## Summary
-LP withdrawals can be blocked by a malicious actor inflating the OI to intentionally increase `unlocked_reserves`.
-
-## Vulnerability Detail
-The protocol locks user deposits in order to ensure future payout obligations can be met, a key functionality outlined in the comment specifications. The `pool.vy` function [`unlocked_reserves`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L125-L130) reports the token reserves not currently tied up to fulfill future payouts:
-
-```vyper
-def unlocked_reserves(id: uint256) -> Tokens:
- ps: PoolState = Pools(self).lookup(id)
- return Tokens({
- base : ps.base_reserves - ps.base_interest,
- quote: ps.quote_reserves - ps.quote_interest,
- })
-```
-
-This function is mentioned in [`calc_burn`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L216-L222) which is called when LPs are making a withdrawal:
-
-```vyper
- unlocked: Tokens = Pools(self).unlocked_reserves(id)
- ...
- assert uv >= bv, ERR_PRECONDITIONS
- assert amts.base <= unlocked.base, ERR_PRECONDITIONS
- assert amts.quote <= unlocked.quote, ERR_PRECONDITIONS
-```
-
-Additionally, after every user action in `core.vy` (including burn and open) an [invariants](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/core.vy#L126-L133) check is performed with similar enforcement:
+@internal
+@pure
+def apply(x: uint256, numerator: uint256) -> uint256:
+ """
+ Fees are represented as numerator only, with the denominator defined
+ here. This computes x*fee capped at x.
+ """
+ fee : uint256 = (x * numerator) / DENOM
+ fee_ : uint256 = fee if fee <= x else x
+ return fee_
-```vyper
-def INVARIANTS(id: uint256, base_token: address, quote_token: address):
- pool : PoolState = self.POOLS.lookup(id)
-...
- assert pool.base_reserves >= pool.base_interest, ERR_INVARIANTS
- assert pool.quote_reserves >= pool.quote_interest, ERR_INVARIANTS
-```
+@internal
+@pure
+def apply2(x: uint256, numerator: uint256) -> uint256:
+ """
+ Fees are represented as numerator only, with the denominator defined
+ here. This computes x*fee capped at x.
+ """
+ fee : uint256 = (x * numerator) / DENOM2
+ fee_ : uint256 = fee if fee <= x else x
+ return fee_
-Critically, the same comparison between `interest` and `reserves` is being made in both cases. Since `open` increases the `interest`, a malicious attacker can take out positions with large enough size and leverage to decrease `unlocked_reserves` to an arbitrarily small value.
+@internal
+@pure
+def divide(paid: uint256, collateral: uint256) -> uint256:
+ if collateral == 0: return 0
+ else : return (paid * ZEROS) / collateral
-As long as he is well-capitalized, he can take out equal notional size positions on both the long and short side, eliminating price fluctuations and funding payments from the cost of this attack, and locking both quote and base reserves. While he would still pay borrowing rates, as well as the initial cost of opening a position, these costs are low enough to enable him to maintain this DoS (assuming he manages his position periodically to avoid liquidation).
+@internal
+@pure
+def multiply(ci: uint256, terms: uint256) -> uint256:
+ return (ci * terms) / ZEROS
-## Impact
-With a large enough bankroll, all withdrawals can be blocked for several months at a nominal cost to the attacker. Anytime the unlocked reserves grows as a result of user actions (eg. mint, close), he can simply open more positions to extend the attack.
+@internal
+@pure
+def slice(y_i: uint256, y_j: uint256) -> uint256:
+ return y_j - y_i
-Withdrawals can very easily be blocked for a period of 4 hours (DoS period mentioned in the [contest rules](https://audits.sherlock.xyz/contests/526)) with a much more modest bankroll and cost of attack.
+@external
+@pure
+def utilization(reserves: uint256, interest: uint256) -> uint256:
+ """
+ Reserve utilization in percent (rounded down). @audit this is actually rounded up...
+ """
+ return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
-Opening new positions will also be blocked, but the blocking of withdrawals is much more problematic as this is a time-sensitive operation given fluctuating crypto prices and user liquidity needs (especially if the attack is carried out for the long durations described). This would significantly erode trust in the protocol and discourage future depositors, even after the attack has concluded.
+@external
+@pure
+def utilization2(reserves: uint256, interest: uint256) -> uint256:
+ """
+ Reserve utilization in percent (rounded down). @audit this is actually rounded up...
+ """
+ return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100_000))
-## Code Snippet
+@external
+@pure
+def scale(fee: uint256, utilization: uint256) -> uint256:
+ return (fee * utilization) / 100
-## Tool used
+@external
+@pure
+def scale2(fee: uint256, utilization: uint256) -> uint256:
+ return (fee * utilization) / 100_000
-Manual Review
+@internal
+@view
+def current_fees() -> FeeState:
+ """
+ Update incremental fee state, called whenever the pool state changes.
+ """
+ # prev/last updated state
+ fs : FeeState = self.lookupFees()
+ # current state
+ ps : PoolState = self.lookup()
+ new_fees : DynFees = self.dynamic_fees()
+ # number of blocks elapsed
+ new_terms: uint256 = block.number - fs.t1
-## Recommendation
-Specifically for the opening of positions, enforce a lower limit of accessible reserves in order to facilitate withdrawals even when positions can no longer be opened.
+ funding_long_sum : uint256 = self.extend(fs.funding_long_sum, fs.funding_long, new_terms)
+ funding_short_sum : uint256 = self.extend(fs.funding_short_sum, fs.funding_short, new_terms)
-For example at the end of opening a position, consider asserting:
-```vyper
- assert pool.base_reserves >= 2*pool.base_interest, ERR_INVARIANTS
- assert pool.quote_reserves >= 2*pool.quote_interest, ERR_INVARIANTS
-```
+ paid_long_term : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
+ received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
-# Issue M-8: Loss of Funds and Delayed Liquidations Due to Inaccurate Fee Accrual
+ paid_short_term : uint256 = self.apply(fs.short_collateral, fs.funding_short * new_terms)
+ received_long_term : uint256 = self.divide(paid_short_term, fs.long_collateral)
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/38
+ received_long_sum : uint256 = self.extend(fs.received_long_sum, received_long_term, 1)
+ received_short_sum : uint256 = self.extend(fs.received_short_sum, received_short_term, 1)
-## Found by
-0xbranded
-## Summary
-All time-dependent fees in the system accrue according to an inaccurate approximation that will progressively lead to significant deviations from their implied cumulative value. This applies to both long/short rates, with both funding paid/received and borrowing rates impacted. As a result, liquidations will be delayed and LPs/funding recipients will be significantly underpaid.
+ if new_terms == 0:
+ return FeeState({
+ t1 : fs.t1,
+ funding_long : new_fees.funding_long,
+ funding_short : new_fees.funding_short,
+ long_collateral : ps.quote_collateral,
+ short_collateral : ps.base_collateral,
+ funding_long_sum : fs.funding_long_sum,
+ funding_short_sum : fs.funding_short_sum,
+ received_long_sum : fs.received_long_sum,
+ received_short_sum : fs.received_short_sum,
+ })
+ else:
+ return FeeState({
+ t1 : block.number,
+ funding_long : new_fees.funding_long,
+ funding_short : new_fees.funding_short,
+ long_collateral : ps.quote_collateral,
+ short_collateral : ps.base_collateral,
+ funding_long_sum : funding_long_sum,
+ funding_short_sum : funding_short_sum,
+ received_long_sum : received_long_sum,
+ received_short_sum : received_short_sum,
+ })
-## Vulnerability Detail
-Each [fee calculation](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/fees.vy#L164-L193) in `fees.vy`, such as
+@internal
+@view
+def current_fees2() -> FeeState:
+ """
+ Update incremental fee state, called whenever the pool state changes.
+ """
+ # prev/last updated state
+ fs : FeeState = self.lookupFees2()
+ # current state
+ ps : PoolState = self.lookup()
+ new_fees : DynFees = self.dynamic_fees()
+ # number of blocks elapsed
+ new_terms: uint256 = block.number - fs.t1
+ funding_long_sum : uint256 = self.extend(fs.funding_long_sum, fs.funding_long, new_terms)
+ funding_short_sum : uint256 = self.extend(fs.funding_short_sum, fs.funding_short, new_terms)
-```vyper
-borrowing_long_sum : uint256 = self.extend(fs.borrowing_long_sum, fs.borrowing_long, new_terms)
-borrowing_short_sum : uint256 = self.extend(fs.borrowing_short_sum, fs.borrowing_short, new_terms)
-funding_long_sum : uint256 = self.extend(fs.funding_long_sum, fs.funding_long, new_terms)
-funding_short_sum : uint256 = self.extend(fs.funding_short_sum, fs.funding_short, new_terms)
-```
+ paid_long_term : uint256 = self.apply2(fs.long_collateral, fs.funding_long * new_terms)
+ received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
-makes use of the [same underlying calculation](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/fees.vy#L94-L98) through the use of `extend`:
+ paid_short_term : uint256 = self.apply2(fs.short_collateral, fs.funding_short * new_terms)
+ received_long_term : uint256 = self.divide(paid_short_term, fs.long_collateral)
-```vyper
-def extend(X: uint256, x_m: uint256, m: uint256) -> uint256:
- return X + (m*x_m)
-```
+ received_long_sum : uint256 = self.extend(fs.received_long_sum, received_long_term, 1)
+ received_short_sum : uint256 = self.extend(fs.received_short_sum, received_short_term, 1)
-That is, given a rate `r` and a number of intervals `n`, the fee contribution of each interval is given by $nr$, and the overall rate accrues as $\sum\limits_i n_ir_i$. However, this calculation is problematic since the percentage rates are naively added up and applied entirely at once, rather than applying them continuously.
+ if new_terms == 0:
+ return FeeState({
+ t1 : fs.t1,
+ funding_long : new_fees.funding_long,
+ funding_short : new_fees.funding_short,
+ long_collateral : ps.quote_collateral,
+ short_collateral : ps.base_collateral,
+ funding_long_sum : fs.funding_long_sum,
+ funding_short_sum : fs.funding_short_sum,
+ received_long_sum : fs.received_long_sum,
+ received_short_sum : fs.received_short_sum,
+ })
+ else:
+ return FeeState({
+ t1 : block.number,
+ funding_long : new_fees.funding_long,
+ funding_short : new_fees.funding_short,
+ long_collateral : ps.quote_collateral,
+ short_collateral : ps.base_collateral,
+ funding_long_sum : funding_long_sum,
+ funding_short_sum : funding_short_sum,
+ received_long_sum : received_long_sum,
+ received_short_sum : received_short_sum,
+ })
-The exact fee calculation over each interval is given $(1+r)^n$ (now expressed as a ratio of 1), and the overall rate accrues as $\prod\limits_i (1 + {r_i})^{n_i}$. The Taylor expansion of each segment of this polynomial is given by $(1+r)^n = 1 + nr + \frac{1}{2}(n-1)nr^2 + \frac{1}{6}(n-2)(n-1)nr^3 + \cdots$
+@internal
+@view
+def query(opened_at: uint256) -> Period:
+ """
+ Return the total fees due from block `opened_at` to the current block.
+ """
+ fees_i : FeeState = self.fees_at_block(opened_at)
+ fees_j : FeeState = self.current_fees()
+ return Period({
+ funding_long : self.slice(fees_i.funding_long_sum, fees_j.funding_long_sum),
+ funding_short : self.slice(fees_i.funding_short_sum, fees_j.funding_short_sum),
+ received_long : self.slice(fees_i.received_long_sum, fees_j.received_long_sum),
+ received_short : self.slice(fees_i.received_short_sum, fees_j.received_short_sum),
+ })
-Notice that the existing calculation is effectively the linear taylor approximation for the true rate:
-$(1+r)^n = 1 + nr +O(r^2)$
+@external
+@view
+def calc(long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
+ period: Period = self.query(opened_at)
+ P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
+ self.apply(collateral, period.funding_short) )
+ R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
+ self.multiply(collateral, period.received_short) )
-The error is bounded above by $R_2(r) \leq \frac{n(n-1)(1+r)^{n-2}}{2}r^2$. Critically, this approximation is relatively accurate **assuming n and r are not too large**. This assumption does not hold.
+ return SumFees({funding_paid: P_f, funding_received: R_f})
-Starting with `r`, fees are applied with 9 decimals of precision:
+@internal
+@view
+def query2(opened_at: uint256) -> Period:
+ """
+ Return the total fees due from block `opened_at` to the current block.
+ """
+ fees_i : FeeState = self.fees_at_block2(opened_at)
+ fees_j : FeeState = self.current_fees2()
+ return Period({
+ funding_long : self.slice(fees_i.funding_long_sum, fees_j.funding_long_sum),
+ funding_short : self.slice(fees_i.funding_short_sum, fees_j.funding_short_sum),
+ received_long : self.slice(fees_i.received_long_sum, fees_j.received_long_sum),
+ received_short : self.slice(fees_i.received_short_sum, fees_j.received_short_sum),
+ })
-```vyper
-DENOM: constant(uint256) = 1_000_000_000
+@external
+@view
+def calc2(long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
+ period: Period = self.query2(opened_at)
+ P_f : uint256 = self.apply2(collateral, period.funding_long) if long else (
+ self.apply2(collateral, period.funding_short) )
+ R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
+ self.multiply(collateral, period.received_short) )
-def apply(x: uint256, numerator: uint256) -> Fee:
- fee : uint256 = (x * numerator) / DENOM
+ return SumFees({funding_paid: P_f, funding_received: R_f})
```
-These fees are applied on a per-block basis. The BOB chain has an average block time of [2.0 seconds](https://explorer.gobob.xyz/). Using the existing fee calculation, `r = 10` will translate into an 8 hour funding rate of 0.0288%, or an annualized rate of 15.78%, a suitable estimate for what might be expected.
-
-Using the average block time of 2.0 seconds, we find that `n = 1.58e7` blocks in a 365 day calendar year.
-
-Given these values, the existing fee calculation $1 + nr$ yields a 15.78% annual interest rate while the exact value $(1+r)^n$ yields a 17.09% annual interest rate. This represents a 7.68% error in the interest rate over a single year.
-
-As n grows larger, the error continues to grow. Over a 4 year period, `n = 6.31e7` blocks and using the same `r = 10` from before we get interest rates of 63.12% and 83.03% for the current approach and the exact calculation, respectively. A 28.26% error in the interest rate results over this time period. This error steadily converges to 100%, with a **99.7% error over a 30 year period**.
-
-The assumption regarding the average value of `r = 10` (0.0288% 8 hour rate) was made based on [past funding rates for BTC](https://www.coinglass.com/funding/BTC). The funding rate over an 8 hour period has ranged between 0.01% and 0.25% over the past year. Given varying market conditions, as well as differing risk profiles for altcoins vs BTC, this is considered a fair and conservative estimate on average. Even if the assumed interest rate were halved (`r = 5`, 0.0144% 8 hour rate), errors of 3.9% and 15.0% occur over a 1 and 4 year period, respectively. On the other hand if doubled (`r = 20`, 0.0576% 8 hour rate), errors of 15% and 50% occur over the same respective timespans.
-
-Note that during periods of elevated rates, the errors described will grow non-linearly. For example, given `r = 100` (0.288% 8 hour rate), the existing method will yield an effective annual rate of 157.8%. The exact calculation yields a rate of 384.5%. While these rates are not representative of typical market conditions, it should paint a picture of how drastically inaccurate the current calculation could be, depending on the circumstances. The assumptions made above should serve as a rough estimate of average conditions, however periods with rates above this average are weighted more heavily than periods with lower rates.
-
-## Impact
-Any Taylor approximation of the true rate is a strict underestimate. As a result, significant underpayments of funding fees and borrowing fees will consistently occur for every pool/position, disincentivizing LPs and carry traders and eroding confidence/trust in the system. Further, positions paying funding fees (historically longs) are effectively subsidized under the current model, increasing utilization and reducing capital efficiency. These positions (typically longs) will also take longer to liquidate and LP funds are locked for longer as a result.
-
-As a rough demonstration, consider a pool with an average long collateral value of $10M over a 4-year period, with an average 8-hour long funding rate of 0.0288% (as assumed above). Over this time period, longs are expected to pay $8.8M in funding fees to shorts. In reality they will pay $6.3M, **representing a shortfall of $2.5M** that should have been deducted from longs and paid to shorts.
-
-Assuming a 50% imbalance between shorts and longs, the average annual borrow rate is 31.56%, so LPs for this pool will also be underpaid by $5.0M from longs, in addition to a similar fee underpayments from shorts.
-
-Under this scenario, longs would have taken 25 months to liquidate from fees alone, when they should have taken 23 months to liquidate. This 10% deviation in absolute liquidation time underscores the true difference in liquidation time. When PnL/volatility are taken into account, these differences at the margin make all the difference and can result in LP funds being locked for far longer than this difference suggests. Additionally, positions which should have been liquidated may now post a profit, further increasing risk for LPs.
-
-These underpayments occur persistently, at all times and for every vault, and represent material losses for funding recipients and lenders, as well as significantly disrupt the proper liquidation process.
-
-## Code Snippet
-For the PoC, `r = 10`, `long_collateral = short_collateral = 10^7 * 10^18` and `n = 6.3 * 10^7` blocks (4 years), as outlined above.
-
-The smart contracts were stripped to isolate the relevant logic, and [foundry](https://github.com/0xKitsune/Foundry-Vyper) was used for testing. To run the test, clone the repo and place `Fee.vy` in `vyper_contracts`, and place `Fee.t.sol`, `Cheat.sol`, and `IFee.sol` under `src/test`.
+
-Fee.t.sol
+Denom.t.sol
```solidity
-
// SPDX-License-Identifier: MIT
pragma solidity >=0.8.13;
@@ -473,3221 +446,2490 @@ import "../../lib/ds-test/test.sol";
import "../../lib/utils/Console.sol";
import "../../lib/utils/VyperDeployer.sol";
-import "./IFee.sol";
+import "../IDenom.sol";
-contract FeeTest is DSTest {
+contract DenomTest is DSTest {
///@notice create a new instance of VyperDeployer
VyperDeployer vyperDeployer = new VyperDeployer();
CheatCodes vm = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
- IFee fee;
+ IDenom denom;
uint256 constant YEAR = (60*60*24*(365) + 60*60*24 / 4); //includes leap year
function setUp() public {
+ vm.roll(1);
///@notice deploy a new instance of ISimplestore by passing in the address of the deployed Vyper contract
- fee = IFee(vyperDeployer.deployContract("Fee"));
+ denom = IDenom(vyperDeployer.deployContract("Denom"));
}
- function testCalc() public {
- uint256 blocksIn4Yr = (4*YEAR) / 2;
+ function testDenom() public {
+ uint256 blocksInYr = (YEAR) / 2;
- vm.roll(blocksIn4Yr);
+ vm.roll(blocksInYr);
- fee.update();
+ denom.update();
// pools were initialized at block 0
- fee.calc(true, 1e25, 0); // (current) linear fee approximation
- fee.calc2(true, 1e25, 0); // quadratic approximation
- fee.calc3(true, 1e25, 0); // cubic approximation
+ denom.calc(true, 1e25, 0);
+ denom.calc2(true, 1e25, 0);
}
-}
-```
-
-
- Fee.vy
+ function testRounding() public {
+ uint max_fee = 100; // set max 8-hour rate to 0.288% (157.8% annually)
-```vyper
+ uint256 reserves = 10_000_000 ether;
+ uint256 interest1 = 1_000_000 ether;
+ uint256 interest2 = 1_099_999 ether;
-struct DynFees:
- funding_long : uint256
- funding_short : uint256
+ uint256 util1 = denom.utilization(reserves, interest1);
+ uint256 util2 = denom.utilization(reserves, interest2);
-struct PoolState:
- base_collateral : uint256
- quote_collateral : uint256
+ assertEq(util1, util2); // current calculation yields same utilization for both interests
-struct FeeState:
- t1 : uint256
- funding_long : uint256
- funding_short : uint256
- long_collateral : uint256
- short_collateral : uint256
- funding_long_sum : uint256
- funding_short_sum : uint256
- received_long_sum : uint256
- received_short_sum : uint256
+ // borrow rate
+ uint fee1 = denom.scale(max_fee, util1);
+ uint fee2 = denom.scale(max_fee, util2);
-struct SumFees:
- funding_paid : uint256
- funding_received: uint256
+ assertEq(fee1, fee2); // results in the same fee also. fee2 should be ~1% higher
+ }
-struct Period:
- funding_long : uint256
- funding_short : uint256
- received_long : uint256
- received_short : uint256
+ function testCombined() public {
+ // now let's see what would happen if we raised the precision of both fees and percents
+ uint max_fee = 65;
+ uint max_fee2 = 65_000; // 3 extra digits of precision lowers error by 3 orders of magnitude
-#starting point hardcoded
-@external
-def __init__():
- self.FEE_STORE = FeeState({
- t1 : block.number,
- funding_long : 10,
- funding_short : 0,
- long_collateral : 10_000_000_000_000_000_000_000_000,
- short_collateral : 10_000_000_000_000_000_000_000_000,
- funding_long_sum : 0,
- funding_short_sum : 0,
- received_long_sum : 0,
- received_short_sum : 0,
- })
+ uint256 reserves = 10_000_000 ether;
+ uint256 interest = 199_999 ether; // interest & reserves same in both, only differ in precision.
- self.FEE_STORE_AT[block.number] = self.FEE_STORE
+ uint256 util1 = denom.utilization(reserves, interest);
+ uint256 util2 = denom.utilization2(reserves, interest); // 3 extra digits of precision here also
+
+ // borrow rate
+ uint fee1 = denom.scale(max_fee, util1);
+ uint fee2 = denom.scale2(max_fee2, util2);
- self.FEE_STORE2 = self.FEE_STORE
- self.FEE_STORE_AT2[block.number] = self.FEE_STORE
- self.FEE_STORE3 = self.FEE_STORE
- self.FEE_STORE_AT3[block.number] = self.FEE_STORE
+ assertEq(fee1 * 1_000, fee2 - 999); // fee 1 is 1.000, fee 2 is 1.999 (~50% error)
+ }
+}
-# hardcoded funding rates for the scenario where funding is positive
-@internal
-@view
-def dynamic_fees() -> DynFees:
- return DynFees({
- funding_long : 10,
- funding_short : 0,
- })
+```
-# #hardcoded pool to have 1e24 of quote and base collateral
-@internal
-@view
-def lookup() -> PoolState:
- return PoolState({
- base_collateral : 10_000_000_000_000_000_000_000_000,
- quote_collateral : 10_000_000_000_000_000_000_000_000,
- })
+
+
+Cheat.sol
+```solidity
+interface CheatCodes {
+ // This allows us to getRecordedLogs()
+ struct Log {
+ bytes32[] topics;
+ bytes data;
+ }
-FEE_STORE : FeeState
-FEE_STORE_AT : HashMap[uint256, FeeState]
+ // Possible caller modes for readCallers()
+ enum CallerMode {
+ None,
+ Broadcast,
+ RecurrentBroadcast,
+ Prank,
+ RecurrentPrank
+ }
-FEE_STORE2 : FeeState
-FEE_STORE_AT2 : HashMap[uint256, FeeState]
+ enum AccountAccessKind {
+ Call,
+ DelegateCall,
+ CallCode,
+ StaticCall,
+ Create,
+ SelfDestruct,
+ Resume
+ }
-FEE_STORE3 : FeeState
-FEE_STORE_AT3 : HashMap[uint256, FeeState]
+ struct Wallet {
+ address addr;
+ uint256 publicKeyX;
+ uint256 publicKeyY;
+ uint256 privateKey;
+ }
-@internal
-@view
-def lookupFees() -> FeeState:
- return self.FEE_STORE
+ struct ChainInfo {
+ uint256 forkId;
+ uint256 chainId;
+ }
-@internal
-@view
-def lookupFees2() -> FeeState:
- return self.FEE_STORE2
+ struct AccountAccess {
+ ChainInfo chainInfo;
+ AccountAccessKind kind;
+ address account;
+ address accessor;
+ bool initialized;
+ uint256 oldBalance;
+ uint256 newBalance;
+ bytes deployedCode;
+ uint256 value;
+ bytes data;
+ bool reverted;
+ StorageAccess[] storageAccesses;
+ }
-@internal
-@view
-def lookupFees3() -> FeeState:
- return self.FEE_STORE3
+ struct StorageAccess {
+ address account;
+ bytes32 slot;
+ bool isWrite;
+ bytes32 previousValue;
+ bytes32 newValue;
+ bool reverted;
+ }
-@internal
-@view
-def fees_at_block(height: uint256) -> FeeState:
- return self.FEE_STORE_AT[height]
+ // Derives a private key from the name, labels the account with that name, and returns the wallet
+ function createWallet(string calldata) external returns (Wallet memory);
-@internal
-@view
-def fees_at_block2(height: uint256) -> FeeState:
- return self.FEE_STORE_AT2[height]
+ // Generates a wallet from the private key and returns the wallet
+ function createWallet(uint256) external returns (Wallet memory);
-@internal
-@view
-def fees_at_block3(height: uint256) -> FeeState:
- return self.FEE_STORE_AT3[height]
+ // Generates a wallet from the private key, labels the account with that name, and returns the wallet
+ function createWallet(uint256, string calldata) external returns (Wallet memory);
-@external
-def update():
- fs: FeeState = self.current_fees()
- fs2: FeeState = self.current_fees2()
- fs3: FeeState = self.current_fees3()
+ // Signs data, (Wallet, digest) => (v, r, s)
+ function sign(Wallet calldata, bytes32) external returns (uint8, bytes32, bytes32);
- self.FEE_STORE_AT[block.number] = fs
- self.FEE_STORE = fs
+ // Get nonce for a Wallet
+ function getNonce(Wallet calldata) external returns (uint64);
- self.FEE_STORE_AT2[block.number] = fs2
- self.FEE_STORE2 = fs2
+ // Set block.timestamp
+ function warp(uint256) external;
- self.FEE_STORE_AT3[block.number] = fs3
- self.FEE_STORE3 = fs3
+ // Set block.number
+ function roll(uint256) external;
-#math
-ZEROS: constant(uint256) = 1000000000000000000000000000
-DENOM: constant(uint256) = 1_000_000_000
+ // Set block.basefee
+ function fee(uint256) external;
-@internal
-@pure
-def extend(X: uint256, x_m: uint256, m: uint256) -> uint256:
- return X + (m*x_m)
+ // Set block.difficulty
+ // Does not work from the Paris hard fork and onwards, and will revert instead.
+ function difficulty(uint256) external;
+
+ // Set block.prevrandao
+ // Does not work before the Paris hard fork, and will revert instead.
+ function prevrandao(bytes32) external;
-@internal
-@pure
-def o2(r: uint256, n: uint256) -> uint256:
- if(n == 0):
- return 0
- return r*n + ((n-1)*n*(r**2)/2)/DENOM
+ // Set block.chainid
+ function chainId(uint256) external;
-@internal
-@pure
-def o3(r: uint256, n: uint256) -> uint256:
- if(n == 0):
- return 0
- if(n == 1):
- return r*n
+ // Loads a storage slot from an address
+ function load(address account, bytes32 slot) external returns (bytes32);
- return r*n + ((n-1)*n*(r**2)/2)/DENOM + ((n-2)*(n-1)*n*(r**3)/6)/DENOM**2
+ // Stores a value to an address' storage slot
+ function store(address account, bytes32 slot, bytes32 value) external;
-@internal
-@pure
-def apply(x: uint256, numerator: uint256) -> uint256:
- """
- Fees are represented as numerator only, with the denominator defined
- here. This computes x*fee capped at x.
- """
- fee : uint256 = (x * numerator) / DENOM
- fee_ : uint256 = fee if fee <= x else x
- return fee_
+ // Signs data
+ function sign(uint256 privateKey, bytes32 digest)
+ external
+ returns (uint8 v, bytes32 r, bytes32 s);
-@internal
-@pure
-def divide(paid: uint256, collateral: uint256) -> uint256:
- if collateral == 0: return 0
- else : return (paid * ZEROS) / collateral
+ // Computes address for a given private key
+ function addr(uint256 privateKey) external returns (address);
-@internal
-@pure
-def multiply(ci: uint256, terms: uint256) -> uint256:
- return (ci * terms) / ZEROS
+ // Derive a private key from a provided mnemonic string,
+ // or mnemonic file path, at the derivation path m/44'/60'/0'/0/{index}.
+ function deriveKey(string calldata, uint32) external returns (uint256);
+ // Derive a private key from a provided mnemonic string, or mnemonic file path,
+ // at the derivation path {path}{index}
+ function deriveKey(string calldata, string calldata, uint32) external returns (uint256);
-@internal
-@pure
-def slice(y_i: uint256, y_j: uint256) -> uint256:
- return y_j - y_i
+ // Gets the nonce of an account
+ function getNonce(address account) external returns (uint64);
-@internal
-@view
-def current_fees() -> FeeState:
- """
- Update incremental fee state, called whenever the pool state changes.
- """
- # prev/last updated state
- fs : FeeState = self.lookupFees()
- # current state
- ps : PoolState = self.lookup()
- new_fees : DynFees = self.dynamic_fees()
- # number of blocks elapsed
- new_terms: uint256 = block.number - fs.t1
+ // Sets the nonce of an account
+ // The new nonce must be higher than the current nonce of the account
+ function setNonce(address account, uint64 nonce) external;
- funding_long_sum : uint256 = self.extend(fs.funding_long_sum, fs.funding_long, new_terms)
- funding_short_sum : uint256 = self.extend(fs.funding_short_sum, fs.funding_short, new_terms)
+ // Performs a foreign function call via terminal
+ function ffi(string[] calldata) external returns (bytes memory);
- paid_long_term : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
- received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
+ // Set environment variables, (name, value)
+ function setEnv(string calldata, string calldata) external;
- paid_short_term : uint256 = self.apply(fs.short_collateral, fs.funding_short * new_terms)
- received_long_term : uint256 = self.divide(paid_short_term, fs.long_collateral)
+ // Read environment variables, (name) => (value)
+ function envBool(string calldata) external returns (bool);
+ function envUint(string calldata) external returns (uint256);
+ function envInt(string calldata) external returns (int256);
+ function envAddress(string calldata) external returns (address);
+ function envBytes32(string calldata) external returns (bytes32);
+ function envString(string calldata) external returns (string memory);
+ function envBytes(string calldata) external returns (bytes memory);
- received_long_sum : uint256 = self.extend(fs.received_long_sum, received_long_term, 1)
- received_short_sum : uint256 = self.extend(fs.received_short_sum, received_short_term, 1)
+ // Read environment variables as arrays, (name, delim) => (value[])
+ function envBool(string calldata, string calldata)
+ external
+ returns (bool[] memory);
+ function envUint(string calldata, string calldata)
+ external
+ returns (uint256[] memory);
+ function envInt(string calldata, string calldata)
+ external
+ returns (int256[] memory);
+ function envAddress(string calldata, string calldata)
+ external
+ returns (address[] memory);
+ function envBytes32(string calldata, string calldata)
+ external
+ returns (bytes32[] memory);
+ function envString(string calldata, string calldata)
+ external
+ returns (string[] memory);
+ function envBytes(string calldata, string calldata)
+ external
+ returns (bytes[] memory);
- if new_terms == 0:
- return FeeState({
- t1 : fs.t1,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : fs.funding_long_sum,
- funding_short_sum : fs.funding_short_sum,
- received_long_sum : fs.received_long_sum,
- received_short_sum : fs.received_short_sum,
- })
- else:
- return FeeState({
- t1 : block.number,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : funding_long_sum,
- funding_short_sum : funding_short_sum,
- received_long_sum : received_long_sum,
- received_short_sum : received_short_sum,
- })
+ // Read environment variables with default value, (name, value) => (value)
+ function envOr(string calldata, bool) external returns (bool);
+ function envOr(string calldata, uint256) external returns (uint256);
+ function envOr(string calldata, int256) external returns (int256);
+ function envOr(string calldata, address) external returns (address);
+ function envOr(string calldata, bytes32) external returns (bytes32);
+ function envOr(string calldata, string calldata) external returns (string memory);
+ function envOr(string calldata, bytes calldata) external returns (bytes memory);
+
+ // Read environment variables as arrays with default value, (name, value[]) => (value[])
+ function envOr(string calldata, string calldata, bool[] calldata) external returns (bool[] memory);
+ function envOr(string calldata, string calldata, uint256[] calldata) external returns (uint256[] memory);
+ function envOr(string calldata, string calldata, int256[] calldata) external returns (int256[] memory);
+ function envOr(string calldata, string calldata, address[] calldata) external returns (address[] memory);
+ function envOr(string calldata, string calldata, bytes32[] calldata) external returns (bytes32[] memory);
+ function envOr(string calldata, string calldata, string[] calldata) external returns (string[] memory);
+ function envOr(string calldata, string calldata, bytes[] calldata) external returns (bytes[] memory);
+
+ // Convert Solidity types to strings
+ function toString(address) external returns(string memory);
+ function toString(bytes calldata) external returns(string memory);
+ function toString(bytes32) external returns(string memory);
+ function toString(bool) external returns(string memory);
+ function toString(uint256) external returns(string memory);
+ function toString(int256) external returns(string memory);
+
+ // Sets the *next* call's msg.sender to be the input address
+ function prank(address) external;
+
+ // Sets all subsequent calls' msg.sender to be the input address
+ // until `stopPrank` is called
+ function startPrank(address) external;
+
+ // Sets the *next* call's msg.sender to be the input address,
+ // and the tx.origin to be the second input
+ function prank(address, address) external;
+
+ // Sets all subsequent calls' msg.sender to be the input address until
+ // `stopPrank` is called, and the tx.origin to be the second input
+ function startPrank(address, address) external;
+
+ // Resets subsequent calls' msg.sender to be `address(this)`
+ function stopPrank() external;
+
+ // Reads the current `msg.sender` and `tx.origin` from state and reports if there is any active caller modification
+ function readCallers() external returns (CallerMode callerMode, address msgSender, address txOrigin);
+
+ // Sets an address' balance
+ function deal(address who, uint256 newBalance) external;
+
+ // Sets an address' code
+ function etch(address who, bytes calldata code) external;
+
+ // Marks a test as skipped. Must be called at the top of the test.
+ function skip(bool skip) external;
+
+ // Expects an error on next call
+ function expectRevert() external;
+ function expectRevert(bytes calldata) external;
+ function expectRevert(bytes4) external;
+
+ // Record all storage reads and writes
+ function record() external;
+
+ // Gets all accessed reads and write slot from a recording session,
+ // for a given address
+ function accesses(address)
+ external
+ returns (bytes32[] memory reads, bytes32[] memory writes);
+
+ // Record all account accesses as part of CREATE, CALL or SELFDESTRUCT opcodes in order,
+ // along with the context of the calls.
+ function startStateDiffRecording() external;
+
+ // Returns an ordered array of all account accesses from a `startStateDiffRecording` session.
+ function stopAndReturnStateDiff() external returns (AccountAccess[] memory accesses);
+
+ // Record all the transaction logs
+ function recordLogs() external;
+
+ // Gets all the recorded logs
+ function getRecordedLogs() external returns (Log[] memory);
+
+ // Prepare an expected log with the signature:
+ // (bool checkTopic1, bool checkTopic2, bool checkTopic3, bool checkData).
+ //
+ // Call this function, then emit an event, then call a function.
+ // Internally after the call, we check if logs were emitted in the expected order
+ // with the expected topics and data (as specified by the booleans)
+ //
+ // The second form also checks supplied address against emitting contract.
+ function expectEmit(bool, bool, bool, bool) external;
+ function expectEmit(bool, bool, bool, bool, address) external;
+
+ // Mocks a call to an address, returning specified data.
+ //
+ // Calldata can either be strict or a partial match, e.g. if you only
+ // pass a Solidity selector to the expected calldata, then the entire Solidity
+ // function will be mocked.
+ function mockCall(address, bytes calldata, bytes calldata) external;
+
+ // Reverts a call to an address, returning the specified error
+ //
+ // Calldata can either be strict or a partial match, e.g. if you only
+ // pass a Solidity selector to the expected calldata, then the entire Solidity
+ // function will be mocked.
+ function mockCallRevert(address where, bytes calldata data, bytes calldata retdata) external;
+
+ // Clears all mocked and reverted mocked calls
+ function clearMockedCalls() external;
+
+ // Expect a call to an address with the specified calldata.
+ // Calldata can either be strict or a partial match
+ function expectCall(address callee, bytes calldata data) external;
+ // Expect a call to an address with the specified
+ // calldata and message value.
+ // Calldata can either be strict or a partial match
+ function expectCall(address callee, uint256, bytes calldata data) external;
+
+ // Gets the _creation_ bytecode from an artifact file. Takes in the relative path to the json file
+ function getCode(string calldata) external returns (bytes memory);
+ // Gets the _deployed_ bytecode from an artifact file. Takes in the relative path to the json file
+ function getDeployedCode(string calldata) external returns (bytes memory);
+
+ // Label an address in test traces
+ function label(address addr, string calldata label) external;
+
+ // Retrieve the label of an address
+ function getLabel(address addr) external returns (string memory);
+
+ // When fuzzing, generate new inputs if conditional not met
+ function assume(bool) external;
+
+ // Set block.coinbase (who)
+ function coinbase(address) external;
+
+ // Using the address that calls the test contract or the address provided
+ // as the sender, has the next call (at this call depth only) create a
+ // transaction that can later be signed and sent onchain
+ function broadcast() external;
+ function broadcast(address) external;
+
+ // Using the address that calls the test contract or the address provided
+ // as the sender, has all subsequent calls (at this call depth only) create
+ // transactions that can later be signed and sent onchain
+ function startBroadcast() external;
+ function startBroadcast(address) external;
+ function startBroadcast(uint256 privateKey) external;
+
+ // Stops collecting onchain transactions
+ function stopBroadcast() external;
+
+ // Reads the entire content of file to string, (path) => (data)
+ function readFile(string calldata) external returns (string memory);
+ // Get the path of the current project root
+ function projectRoot() external returns (string memory);
+ // Reads next line of file to string, (path) => (line)
+ function readLine(string calldata) external returns (string memory);
+ // Writes data to file, creating a file if it does not exist, and entirely replacing its contents if it does.
+ // (path, data) => ()
+ function writeFile(string calldata, string calldata) external;
+ // Writes line to file, creating a file if it does not exist.
+ // (path, data) => ()
+ function writeLine(string calldata, string calldata) external;
+ // Closes file for reading, resetting the offset and allowing to read it from beginning with readLine.
+ // (path) => ()
+ function closeFile(string calldata) external;
+ // Removes file. This cheatcode will revert in the following situations, but is not limited to just these cases:
+ // - Path points to a directory.
+ // - The file doesn't exist.
+ // - The user lacks permissions to remove the file.
+ // (path) => ()
+ function removeFile(string calldata) external;
+ // Returns true if the given path points to an existing entity, else returns false
+ // (path) => (bool)
+ function exists(string calldata) external returns (bool);
+ // Returns true if the path exists on disk and is pointing at a regular file, else returns false
+ // (path) => (bool)
+ function isFile(string calldata) external returns (bool);
+ // Returns true if the path exists on disk and is pointing at a directory, else returns false
+ // (path) => (bool)
+ function isDir(string calldata) external returns (bool);
+
+ // Return the value(s) that correspond to 'key'
+ function parseJson(string memory json, string memory key) external returns (bytes memory);
+ // Return the entire json file
+ function parseJson(string memory json) external returns (bytes memory);
+ // Check if a key exists in a json string
+ function keyExists(string memory json, string memory key) external returns (bytes memory);
+ // Get list of keys in a json string
+ function parseJsonKeys(string memory json, string memory key) external returns (string[] memory);
+
+ // Snapshot the current state of the evm.
+ // Returns the id of the snapshot that was created.
+ // To revert a snapshot use `revertTo`
+ function snapshot() external returns (uint256);
+ // Revert the state of the evm to a previous snapshot
+ // Takes the snapshot id to revert to.
+ // This deletes the snapshot and all snapshots taken after the given snapshot id.
+ function revertTo(uint256) external returns (bool);
+
+ // Creates a new fork with the given endpoint and block,
+ // and returns the identifier of the fork
+ function createFork(string calldata, uint256) external returns (uint256);
+ // Creates a new fork with the given endpoint and the _latest_ block,
+ // and returns the identifier of the fork
+ function createFork(string calldata) external returns (uint256);
+
+ // Creates _and_ also selects a new fork with the given endpoint and block,
+ // and returns the identifier of the fork
+ function createSelectFork(string calldata, uint256)
+ external
+ returns (uint256);
+ // Creates _and_ also selects a new fork with the given endpoint and the
+ // latest block and returns the identifier of the fork
+ function createSelectFork(string calldata) external returns (uint256);
+
+ // Takes a fork identifier created by `createFork` and
+ // sets the corresponding forked state as active.
+ function selectFork(uint256) external;
+
+ // Returns the currently active fork
+ // Reverts if no fork is currently active
+ function activeFork() external returns (uint256);
+
+ // Updates the currently active fork to given block number
+ // This is similar to `roll` but for the currently active fork
+ function rollFork(uint256) external;
+ // Updates the given fork to given block number
+ function rollFork(uint256 forkId, uint256 blockNumber) external;
+
+ // Fetches the given transaction from the active fork and executes it on the current state
+ function transact(bytes32) external;
+ // Fetches the given transaction from the given fork and executes it on the current state
+ function transact(uint256, bytes32) external;
+
+ // Marks that the account(s) should use persistent storage across
+ // fork swaps in a multifork setup, meaning, changes made to the state
+ // of this account will be kept when switching forks
+ function makePersistent(address) external;
+ function makePersistent(address, address) external;
+ function makePersistent(address, address, address) external;
+ function makePersistent(address[] calldata) external;
+ // Revokes persistent status from the address, previously added via `makePersistent`
+ function revokePersistent(address) external;
+ function revokePersistent(address[] calldata) external;
+ // Returns true if the account is marked as persistent
+ function isPersistent(address) external returns (bool);
+
+ /// Returns the RPC url for the given alias
+ function rpcUrl(string calldata) external returns (string memory);
+ /// Returns all rpc urls and their aliases `[alias, url][]`
+ function rpcUrls() external returns (string[2][] memory);
+}
+```
+
+
+
+
+IDenom.sol
+
+```solidity
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.8.13;
+
+interface IDenom {
+ function update() external;
+ function calc(bool, uint256, uint256) external returns(SumFees memory);
+ function calc2(bool, uint256, uint256) external returns(SumFees memory);
+
+ function utilization(uint256, uint256) external returns(uint256);
+ function utilization2(uint256, uint256) external returns(uint256);
+ function scale(uint256, uint256) external returns(uint256);
+ function scale2(uint256, uint256) external returns(uint256);
+
+ struct SumFees{
+ uint256 funding_paid;
+ uint256 funding_received;
+ }
+}
+```
+
+
+
+## Tool used
+
+Manual Review
+
+## Recommendation
+Consider increasing the precision of `DENOM` by atleast 3 digits, i.e. `DENOM: constant(uint256) = 1_000_000_000_000` instead of `1_000_000_000`. Consider increasing the precision of percentages by 3 digits, i.e. divide / multiply by 100_000 instead of 100.
+
+Each added digit of precision decreases the precision loss by an order of magnitude. In other words the 1% and 1.5% absolute errors in precision would shrink to 0.01% and 0.015% when using three extra digits of precision.
+
+Consult `Denom.vy` for further guidance on necessary adjustments to make to the various functions to account for these updated values.
+
+
+
+## Discussion
+
+**KupiaSecAdmin**
+
+Escalate
+
+This is a system design choice. It just charges less fees, there is no loss of funds happening to the protocol. Also this can be modified by updating parameters.
+
+**sherlock-admin3**
+
+> Escalate
+>
+> This is a system design choice. It just charges less fees, there is no loss of funds happening to the protocol. Also this can be modified by updating parameters.
+
+You've created a valid escalation!
+
+To remove the escalation from consideration: Delete your comment.
+
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
-@internal
-@view
-def current_fees2() -> FeeState:
- """
- Update incremental fee state, called whenever the pool state changes.
- """
- # prev/last updated state
- fs : FeeState = self.lookupFees2()
- # current state
- ps : PoolState = self.lookup()
- new_fees : DynFees = self.dynamic_fees()
- # number of blocks elapsed
- new_terms: uint256 = block.number - fs.t1
+**msheikhattari**
- o2_l: uint256 = self.o2(fs.funding_long, new_terms)
- o2_s: uint256 = self.o2(fs.funding_short, new_terms)
+It's not a design choice, this is clearly a precision loss issue. There are real loss of funds and delayed liquidation that consistently occur given the real world parameters such as blocktime. these are clearly documented in the PoC with specific errors exceeding 10% consistently, and sometimes even total loss of fee payment.
- funding_long_sum : uint256 = self.extend(fs.funding_long_sum, o2_l, 1)
- funding_short_sum : uint256 = self.extend(fs.funding_short_sum, o2_s, 1)
+DENOM is a constant that cannot be updated. It must be corrected by increasing the decimals of precision.
- paid_long_term : uint256 = self.apply(fs.long_collateral, o2_l)
- received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
+Given the high likelihood and high impact of loss of funds not only from fee miscalculation but delayed/avoided liquidations, this is a high severity issue.
- paid_short_term : uint256 = self.apply(fs.short_collateral, o2_s)
- received_long_term : uint256 = self.divide(paid_short_term, fs.long_collateral)
+**KupiaSecAdmin**
- received_long_sum : uint256 = self.extend(fs.received_long_sum, received_long_term, 1)
- received_short_sum : uint256 = self.extend(fs.received_short_sum, received_short_term, 1)
+@msheikhattari - The logic only modifies the fee ratio by rounding it down, if the ratio doesn't meet what the protocol team is looking for, they can simply increase the numerator by updating parameters.
+Also even though with lower fee ratio, why does it cause loss? If borrow fee becomes less, there will be more positions openers who will also increase utilization ratio which also will increase the borrowing fee.
+Still, this is low/info at most.
- if new_terms == 0:
- return FeeState({
- t1 : fs.t1,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : fs.funding_long_sum,
- funding_short_sum : fs.funding_short_sum,
- received_long_sum : fs.received_long_sum,
- received_short_sum : fs.received_short_sum,
- })
- else:
- return FeeState({
- t1 : block.number,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : funding_long_sum,
- funding_short_sum : funding_short_sum,
- received_long_sum : received_long_sum,
- received_short_sum : received_short_sum,
- })
+**msheikhattari**
-@internal
-@view
-def current_fees3() -> FeeState:
- """
- Update incremental fee state, called whenever the pool state changes.
- """
- # prev/last updated state
- fs : FeeState = self.lookupFees3()
- # current state
- ps : PoolState = self.lookup()
- new_fees : DynFees = self.dynamic_fees()
- # number of blocks elapsed
- new_terms: uint256 = block.number - fs.t1
+No parameters can be updated to fix this issue - DENOM must directly be changed (which is a constant). It currently supports annual fees of about 1.5% increments which is way too much precision loss with errors consistently exceeding 10% in all fee calculations.
- o2_l: uint256 = self.o3(fs.funding_long, new_terms)
- o2_s: uint256 = self.o3(fs.funding_short, new_terms)
+The numerator is blocks * rate, the point is that the rate component cannot support fees with a precision below 1.5% because of the DENOM parameter that is too small.
- funding_long_sum : uint256 = self.extend(fs.funding_long_sum, o2_l, 1)
- funding_short_sum : uint256 = self.extend(fs.funding_short_sum, o2_s, 1)
+I included a PoC which clearly demonstrates this currently unavoidable precision loss.
- paid_long_term : uint256 = self.apply(fs.long_collateral, o2_l)
- received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
+**rickkk137**
- paid_short_term : uint256 = self.apply(fs.short_collateral, o2_s)
- received_long_term : uint256 = self.divide(paid_short_term, fs.long_collateral)
+> No parameters can be updated to fix this issue - DENOM must directly be changed (which is a constant). It currently supports annual fees of about 1.5% increments which is way too much precision loss with errors consistently exceeding 10% in all fee calculations.
+>
+> The numerator is blocks * rate, the point is that the rate component cannot support fees with a precision below 1.5% because of the DENOM parameter that is too small.
+>
+> I included a PoC which clearly demonstrates this currently unavoidable precision loss.
- received_long_sum : uint256 = self.extend(fs.received_long_sum, received_long_term, 1)
- received_short_sum : uint256 = self.extend(fs.received_short_sum, received_short_term, 1)
+I read your PoC,root cause in this report is here which mentioned in this report and borrowing_paid calculation doesn't have effect
+```vyper
+def utilization(reserves: uint256, interest: uint256) -> uint256:
+ return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
+```
- if new_terms == 0:
- return FeeState({
- t1 : fs.t1,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : fs.funding_long_sum,
- funding_short_sum : fs.funding_short_sum,
- received_long_sum : fs.received_long_sum,
- received_short_sum : fs.received_short_sum,
- })
- else:
- return FeeState({
- t1 : block.number,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : funding_long_sum,
- funding_short_sum : funding_short_sum,
- received_long_sum : received_long_sum,
- received_short_sum : received_short_sum,
- })
-@internal
-@view
-def query(opened_at: uint256) -> Period:
- """
- Return the total fees due from block `opened_at` to the current block.
- """
- fees_i : FeeState = self.fees_at_block(opened_at)
- fees_j : FeeState = self.current_fees()
- return Period({
- funding_long : self.slice(fees_i.funding_long_sum, fees_j.funding_long_sum),
- funding_short : self.slice(fees_i.funding_short_sum, fees_j.funding_short_sum),
- received_long : self.slice(fees_i.received_long_sum, fees_j.received_long_sum),
- received_short : self.slice(fees_i.received_short_sum, fees_j.received_short_sum),
- })
-@external
-@view
-def calc(long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
- period: Period = self.query(opened_at)
- P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
- self.apply(collateral, period.funding_short) )
- R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
- self.multiply(collateral, period.received_short) )
- return SumFees({funding_paid: P_f, funding_received: R_f})
+**WangSecurity**
-@internal
-@view
-def query2(opened_at: uint256) -> Period:
- """
- Return the total fees due from block `opened_at` to the current block.
- """
- fees_i : FeeState = self.fees_at_block2(opened_at)
- fees_j : FeeState = self.current_fees2()
- return Period({
- funding_long : self.slice(fees_i.funding_long_sum, fees_j.funding_long_sum),
- funding_short : self.slice(fees_i.funding_short_sum, fees_j.funding_short_sum),
- received_long : self.slice(fees_i.received_long_sum, fees_j.received_long_sum),
- received_short : self.slice(fees_i.received_short_sum, fees_j.received_short_sum),
- })
+Though, this may be considered a design decision, the calculation of fees still has precision loss which would pay less fees due to rounding down. Hence, this should've been included in the known issues question in the protocol's README, but it wasn't. Also, `DENOM` couldn't be changed after the contracts were deployed, but it wasn't flagged as a hardcoded variable that could be changed.
-@external
-@view
-def calc2(long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
- period: Period = self.query2(opened_at)
- P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
- self.apply(collateral, period.funding_short) )
- R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
- self.multiply(collateral, period.received_short) )
+Hence, this should remain a valid issue. Planning to reject the escalation.
- return SumFees({funding_paid: P_f, funding_received: R_f})
+**msheikhattari**
-@internal
-@view
-def query3(opened_at: uint256) -> Period:
- """
- Return the total fees due from block `opened_at` to the current block.
- """
- fees_i : FeeState = self.fees_at_block3(opened_at)
- fees_j : FeeState = self.current_fees3()
- return Period({
- funding_long : self.slice(fees_i.funding_long_sum, fees_j.funding_long_sum),
- funding_short : self.slice(fees_i.funding_short_sum, fees_j.funding_short_sum),
- received_long : self.slice(fees_i.received_long_sum, fees_j.received_long_sum),
- received_short : self.slice(fees_i.received_short_sum, fees_j.received_short_sum),
- })
+Given that significant disruptions to liquidations and loss of funds result, this issue should in consideration for high severity.
-@external
-@view
-def calc3(long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
- period: Period = self.query3(opened_at)
- P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
- self.apply(collateral, period.funding_short) )
- R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
- self.multiply(collateral, period.received_short) )
+**WangSecurity**
- return SumFees({funding_paid: P_f, funding_received: R_f})
+Agree with the above, as I understand, there are no extensive limitations, and the loss can be significant. Planning to reject the escalation but make the issue high severity.
+
+**rickkk137**
+
+root cause in this issue and #72 is same
+
+**KupiaSecAdmin**
+
+@WangSecurity - Even though `DENOM` can't be modified, `denominator` and `numerator` are updatable which determines the fee ratio. So making this valid doesn't seem appropriate and it can never be high severity.
+
+**rickkk137**
+
+@KupiaSecAdmin I agree with u ,this issue talks about two part
+first one:
```
-
+def utilization(reserves: uint256, interest: uint256) -> uint256:
+ return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
+```
+second one:
+```
+def apply(x: uint256, numerator: uint256) -> Fee:
+ fee : uint256 = (x * numerator) / DENOM
+ ```
+ first one has precision loss but [second one doesn't have](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/126#issuecomment-2365007747)
-
- Cheat.sol
+**KupiaSecAdmin**
-```solidity
-interface CheatCodes {
- // This allows us to getRecordedLogs()
- struct Log {
- bytes32[] topics;
- bytes data;
- }
+@rickkk137 - Regarding first one, isn't this issue same? https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/87
- // Possible caller modes for readCallers()
- enum CallerMode {
- None,
- Broadcast,
- RecurrentBroadcast,
- Prank,
- RecurrentPrank
- }
+**rickkk137**
- enum AccountAccessKind {
- Call,
- DelegateCall,
- CallCode,
- StaticCall,
- Create,
- SelfDestruct,
- Resume
- }
+@KupiaSecAdmin no,I don't think so, imo #87 is not possible i wrote my comment there
- struct Wallet {
- address addr;
- uint256 publicKeyX;
- uint256 publicKeyY;
- uint256 privateKey;
- }
- struct ChainInfo {
- uint256 forkId;
- uint256 chainId;
- }
+**msheikhattari**
- struct AccountAccess {
- ChainInfo chainInfo;
- AccountAccessKind kind;
- address account;
- address accessor;
- bool initialized;
- uint256 oldBalance;
- uint256 newBalance;
- bytes deployedCode;
- uint256 value;
- bytes data;
- bool reverted;
- StorageAccess[] storageAccesses;
- }
+Please refer to the PoC, the issue is currently unmitigatable due to the precision of all fees that result from the `DENOM` parameter. It doesn't result directly from any of the calculations but the lowest representable precision of fee parameters. Let me know if I can clarify anything further.
- struct StorageAccess {
- address account;
- bytes32 slot;
- bool isWrite;
- bytes32 previousValue;
- bytes32 newValue;
- bool reverted;
- }
+**WangSecurity**
- // Derives a private key from the name, labels the account with that name, and returns the wallet
- function createWallet(string calldata) external returns (Wallet memory);
+I believe @rickkk137 is correct that this and #72 have the same root cause and explain the same problem. The escalation will still be rejected, and #72 + #60 (duplicate) will be duplicated with this report because it goes more in-depth in explaining the issue and shows a higher severity.
- // Generates a wallet from the private key and returns the wallet
- function createWallet(uint256) external returns (Wallet memory);
+**msheikhattari**
- // Generates a wallet from the private key, labels the account with that name, and returns the wallet
- function createWallet(uint256, string calldata) external returns (Wallet memory);
+#72 is describing a different issue. It doesn't mention the DENOM parameter at all and should not be duped with this one.
- // Signs data, (Wallet, digest) => (v, r, s)
- function sign(Wallet calldata, bytes32) external returns (uint8, bytes32, bytes32);
+That issue is talking about setting min values for `long_utilization` and `short_utilization`, the only similarity is that they are both sources of precision loss. Otherwise the underlying issue is different.
- // Get nonce for a Wallet
- function getNonce(Wallet calldata) external returns (uint64);
+**WangSecurity**
- // Set block.timestamp
- function warp(uint256) external;
+Yes, excuse me, focused too much on the utilisation part.
+The precision loss from Denom is relatively low based on the discussion under #126. But here the report combines 2 precision loss factors resulting in a major precision loss. Hence, I'm returning to my previous decision to reject the escalation, increase severity to high and leave it solo, because it combines 2 precision loss factors resulting in a more major issue than #72 which talks only about utilisation. Planning to apply this decision in a couple of hours.
- // Set block.number
- function roll(uint256) external;
+**aslanbekaibimov**
+
+@WangSecurity
+
+> The duplication rules assume we have a "target issue", and the "potential duplicate" of that issue needs to meet the following requirements to be considered a duplicate.
+
+> Identify the root cause
+ Identify at least a Medium impact
+ Identify a valid attack path or vulnerability path
+ Fulfills other submission quality requirements (e.g. provides a PoC for categories that require one)
+
+Don't #72 and #60 satisfy all 4 requirements?
+
+**WangSecurity**
+
+Good question, but #72 and #60 identified only one source of precision loss, so the following should apply:
+
+> The exception to this would be if underlying code implementations OR impact OR the fixes are different, then they may be treated separately.
+
+That's the reason I think they should be treated separately. The decision remains, reject the escalation, increase severity to high.
+
+**rickkk137**
+
+>But here the report combines 2 precision loss factors resulting in a major precision loss
+```
+def utilization(reserves: uint256, interest: uint256) -> uint256:
+ """
+ Reserve utilization in percent (rounded down). @audit this is actually rounded up...
+ """
+ @>>> return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
+
+@external
+@pure
+def utilization2(reserves: uint256, interest: uint256) -> uint256:
+ """
+ Reserve utilization in percent (rounded down). @audit this is actually rounded up...
+ """
+ @>>> return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100_000))
- // Set block.basefee
- function fee(uint256) external;
- // Set block.difficulty
- // Does not work from the Paris hard fork and onwards, and will revert instead.
- function difficulty(uint256) external;
-
- // Set block.prevrandao
- // Does not work before the Paris hard fork, and will revert instead.
- function prevrandao(bytes32) external;
- // Set block.chainid
- function chainId(uint256) external;
+function testCombined() public {
+ // now let's see what would happen if we raised the precision of both fees and percents
+ uint max_fee = 65;
+ uint max_fee2 = 65_000; // 3 extra digits of precision lowers error by 3 orders of magnitude
- // Loads a storage slot from an address
- function load(address account, bytes32 slot) external returns (bytes32);
+ uint256 reserves = 10_000_000 ether;
+ uint256 interest = 199_999 ether; // interest & reserves same in both, only differ in precision.
- // Stores a value to an address' storage slot
- function store(address account, bytes32 slot, bytes32 value) external;
+ uint256 util1 = denom.utilization(reserves, interest);
+ @>>> uint256 util2 = denom.utilization2(reserves, interest); // 3 extra digits of precision here also
+
+ // borrow rate
+ uint fee1 = denom.scale(max_fee, util1);
+ @>>> uint fee2 = denom.scale2(max_fee2, util2);
- // Signs data
- function sign(uint256 privateKey, bytes32 digest)
- external
- returns (uint8 v, bytes32 r, bytes32 s);
+ assertEq(fee1 * 1_000, fee2 - 999); // fee 1 is 1.000, fee 2 is 1.999 (~50% error)
+ }
+```
+the watson passed util2 to denom.scale2 function in his PoC and that made huge difference and utilization2 function is custom function has written by watson
- // Computes address for a given private key
- function addr(uint256 privateKey) external returns (address);
- // Derive a private key from a provided mnemonic string,
- // or mnemonic file path, at the derivation path m/44'/60'/0'/0/{index}.
- function deriveKey(string calldata, uint32) external returns (uint256);
- // Derive a private key from a provided mnemonic string, or mnemonic file path,
- // at the derivation path {path}{index}
- function deriveKey(string calldata, string calldata, uint32) external returns (uint256);
- // Gets the nonce of an account
- function getNonce(address account) external returns (uint64);
- // Sets the nonce of an account
- // The new nonce must be higher than the current nonce of the account
- function setNonce(address account, uint64 nonce) external;
- // Performs a foreign function call via terminal
- function ffi(string[] calldata) external returns (bytes memory);
- // Set environment variables, (name, value)
- function setEnv(string calldata, string calldata) external;
- // Read environment variables, (name) => (value)
- function envBool(string calldata) external returns (bool);
- function envUint(string calldata) external returns (uint256);
- function envInt(string calldata) external returns (int256);
- function envAddress(string calldata) external returns (address);
- function envBytes32(string calldata) external returns (bytes32);
- function envString(string calldata) external returns (string memory);
- function envBytes(string calldata) external returns (bytes memory);
+**msheikhattari**
- // Read environment variables as arrays, (name, delim) => (value[])
- function envBool(string calldata, string calldata)
- external
- returns (bool[] memory);
- function envUint(string calldata, string calldata)
- external
- returns (uint256[] memory);
- function envInt(string calldata, string calldata)
- external
- returns (int256[] memory);
- function envAddress(string calldata, string calldata)
- external
- returns (address[] memory);
- function envBytes32(string calldata, string calldata)
- external
- returns (bytes32[] memory);
- function envString(string calldata, string calldata)
- external
- returns (string[] memory);
- function envBytes(string calldata, string calldata)
- external
- returns (bytes[] memory);
+Yes, these combined sources of precision loss were demonstrated to have a far more problematic impact as shown in the PoC.
- // Read environment variables with default value, (name, value) => (value)
- function envOr(string calldata, bool) external returns (bool);
- function envOr(string calldata, uint256) external returns (uint256);
- function envOr(string calldata, int256) external returns (int256);
- function envOr(string calldata, address) external returns (address);
- function envOr(string calldata, bytes32) external returns (bytes32);
- function envOr(string calldata, string calldata) external returns (string memory);
- function envOr(string calldata, bytes calldata) external returns (bytes memory);
-
- // Read environment variables as arrays with default value, (name, value[]) => (value[])
- function envOr(string calldata, string calldata, bool[] calldata) external returns (bool[] memory);
- function envOr(string calldata, string calldata, uint256[] calldata) external returns (uint256[] memory);
- function envOr(string calldata, string calldata, int256[] calldata) external returns (int256[] memory);
- function envOr(string calldata, string calldata, address[] calldata) external returns (address[] memory);
- function envOr(string calldata, string calldata, bytes32[] calldata) external returns (bytes32[] memory);
- function envOr(string calldata, string calldata, string[] calldata) external returns (string[] memory);
- function envOr(string calldata, string calldata, bytes[] calldata) external returns (bytes[] memory);
+**WangSecurity**
- // Convert Solidity types to strings
- function toString(address) external returns(string memory);
- function toString(bytes calldata) external returns(string memory);
- function toString(bytes32) external returns(string memory);
- function toString(bool) external returns(string memory);
- function toString(uint256) external returns(string memory);
- function toString(int256) external returns(string memory);
+But, as I understand from @rickkk137, his main point is without the precision inside the Utilisation, the loss would be small and not sufficient for medium severity. Is it correct @rickkk137 ?
- // Sets the *next* call's msg.sender to be the input address
- function prank(address) external;
+**rickkk137**
- // Sets all subsequent calls' msg.sender to be the input address
- // until `stopPrank` is called
- function startPrank(address) external;
+Yes,correct
- // Sets the *next* call's msg.sender to be the input address,
- // and the tx.origin to be the second input
- function prank(address, address) external;
+**msheikhattari**
- // Sets all subsequent calls' msg.sender to be the input address until
- // `stopPrank` is called, and the tx.origin to be the second input
- function startPrank(address, address) external;
+The precision loss resulting from DENOM is more significant than that from the utilization. The PoC and described scenario in the issue provides exact details on the loss of each.
- // Resets subsequent calls' msg.sender to be `address(this)`
- function stopPrank() external;
+When combined, not only is the precision loss more severe but also more likely to occur.
- // Reads the current `msg.sender` and `tx.origin` from state and reports if there is any active caller modification
- function readCallers() external returns (CallerMode callerMode, address msgSender, address txOrigin);
+**WangSecurity**
- // Sets an address' balance
- function deal(address who, uint256 newBalance) external;
-
- // Sets an address' code
- function etch(address who, bytes calldata code) external;
+But as I see in #126, the precision loss from Denom is not that severe, is it wrong?
- // Marks a test as skipped. Must be called at the top of the test.
- function skip(bool skip) external;
+**msheikhattari**
- // Expects an error on next call
- function expectRevert() external;
- function expectRevert(bytes calldata) external;
- function expectRevert(bytes4) external;
+That issue is slightly different, but what was pointed out here is that the most granular annual fee representable is about 1.6% - these are the intervals for fee rates as well (ex. 2x 1.6, 3x 1.6...)
- // Record all storage reads and writes
- function record() external;
+Utilization on the other hand experiences precision loss of 1% in the extreme case (ex 14.9% -> 14%)
- // Gets all accessed reads and write slot from a recording session,
- // for a given address
- function accesses(address)
- external
- returns (bytes32[] memory reads, bytes32[] memory writes);
-
- // Record all account accesses as part of CREATE, CALL or SELFDESTRUCT opcodes in order,
- // along with the context of the calls.
- function startStateDiffRecording() external;
+So in absolute terms the issue arising from DENOM is more significant, when combined these issues become far more significant than implied by their nominal values, not only due to multiplied loss of precision but increased likelihood of loss (if precision loss from one source bumps it just over the boundary of another, as outlined in the PoC)
- // Returns an ordered array of all account accesses from a `startStateDiffRecording` session.
- function stopAndReturnStateDiff() external returns (AccountAccess[] memory accesses);
+**WangSecurity**
- // Record all the transaction logs
- function recordLogs() external;
+Yeah, I see. #126 tries to show a scenario where DENOM precision loss would round down the fees to 0, and for that to happen, the fees or collateral have to be very small, which results in a very small loss. But this issue just shows the precision loss from DENOM and doesn't try to show rounding down to 0. That's the key difference between the two reports.
- // Gets all the recorded logs
- function getRecordedLogs() external returns (Log[] memory);
+Hence, my decision remains that this will remain solo with high severity as expressed above. Planning to reject the escalation. The decision will be applied tomorrow at 10 am UTC:
+> *Note: #126 won't be duplicated with this report as it doesn't show Medium impact*
- // Prepare an expected log with the signature:
- // (bool checkTopic1, bool checkTopic2, bool checkTopic3, bool checkData).
- //
- // Call this function, then emit an event, then call a function.
- // Internally after the call, we check if logs were emitted in the expected order
- // with the expected topics and data (as specified by the booleans)
- //
- // The second form also checks supplied address against emitting contract.
- function expectEmit(bool, bool, bool, bool) external;
- function expectEmit(bool, bool, bool, bool, address) external;
+**WangSecurity**
- // Mocks a call to an address, returning specified data.
- //
- // Calldata can either be strict or a partial match, e.g. if you only
- // pass a Solidity selector to the expected calldata, then the entire Solidity
- // function will be mocked.
- function mockCall(address, bytes calldata, bytes calldata) external;
+Result:
+High
+Unique
- // Reverts a call to an address, returning the specified error
- //
- // Calldata can either be strict or a partial match, e.g. if you only
- // pass a Solidity selector to the expected calldata, then the entire Solidity
- // function will be mocked.
- function mockCallRevert(address where, bytes calldata data, bytes calldata retdata) external;
+**sherlock-admin2**
- // Clears all mocked and reverted mocked calls
- function clearMockedCalls() external;
+Escalations have been resolved successfully!
- // Expect a call to an address with the specified calldata.
- // Calldata can either be strict or a partial match
- function expectCall(address callee, bytes calldata data) external;
- // Expect a call to an address with the specified
- // calldata and message value.
- // Calldata can either be strict or a partial match
- function expectCall(address callee, uint256, bytes calldata data) external;
+Escalation status:
+- [KupiaSecAdmin](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/66/#issuecomment-2345173630): rejected
- // Gets the _creation_ bytecode from an artifact file. Takes in the relative path to the json file
- function getCode(string calldata) external returns (bytes memory);
- // Gets the _deployed_ bytecode from an artifact file. Takes in the relative path to the json file
- function getDeployedCode(string calldata) external returns (bytes memory);
+# Issue H-2: User can sandwich their own position close to get back all of their position fees
- // Label an address in test traces
- function label(address addr, string calldata label) external;
-
- // Retrieve the label of an address
- function getLabel(address addr) external returns (string memory);
+Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/94
- // When fuzzing, generate new inputs if conditional not met
- function assume(bool) external;
+## Found by
+0x37, KupiaSec, bughuntoor
+## Summary
+User can sandwich their own position close to get back all of their position fees
- // Set block.coinbase (who)
- function coinbase(address) external;
+## Vulnerability Detail
+Within the protocol, borrowing fees are only distributed to LP providers when the position is closed. Up until then, they remain within the position.
+The problem is that in this way, fees are distributed evenly to LP providers, without taking into account the longevity of their LP provision.
- // Using the address that calls the test contract or the address provided
- // as the sender, has the next call (at this call depth only) create a
- // transaction that can later be signed and sent onchain
- function broadcast() external;
- function broadcast(address) external;
+This allows a user to avoid paying fees in the following way:
+1. Flashloan a large sum and add it as liquidity
+2. Close their position and let the fees be distributed (with most going back to them as they've got majority in the pool)
+3. WIthdraw their LP tokens
+4. Pay back the flashloan
- // Using the address that calls the test contract or the address provided
- // as the sender, has all subsequent calls (at this call depth only) create
- // transactions that can later be signed and sent onchain
- function startBroadcast() external;
- function startBroadcast(address) external;
- function startBroadcast(uint256 privateKey) external;
+## Impact
+Users can avoid paying borrowing fees.
- // Stops collecting onchain transactions
- function stopBroadcast() external;
+## Code Snippet
+https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L156
- // Reads the entire content of file to string, (path) => (data)
- function readFile(string calldata) external returns (string memory);
- // Get the path of the current project root
- function projectRoot() external returns (string memory);
- // Reads next line of file to string, (path) => (line)
- function readLine(string calldata) external returns (string memory);
- // Writes data to file, creating a file if it does not exist, and entirely replacing its contents if it does.
- // (path, data) => ()
- function writeFile(string calldata, string calldata) external;
- // Writes line to file, creating a file if it does not exist.
- // (path, data) => ()
- function writeLine(string calldata, string calldata) external;
- // Closes file for reading, resetting the offset and allowing to read it from beginning with readLine.
- // (path) => ()
- function closeFile(string calldata) external;
- // Removes file. This cheatcode will revert in the following situations, but is not limited to just these cases:
- // - Path points to a directory.
- // - The file doesn't exist.
- // - The user lacks permissions to remove the file.
- // (path) => ()
- function removeFile(string calldata) external;
- // Returns true if the given path points to an existing entity, else returns false
- // (path) => (bool)
- function exists(string calldata) external returns (bool);
- // Returns true if the path exists on disk and is pointing at a regular file, else returns false
- // (path) => (bool)
- function isFile(string calldata) external returns (bool);
- // Returns true if the path exists on disk and is pointing at a directory, else returns false
- // (path) => (bool)
- function isDir(string calldata) external returns (bool);
-
- // Return the value(s) that correspond to 'key'
- function parseJson(string memory json, string memory key) external returns (bytes memory);
- // Return the entire json file
- function parseJson(string memory json) external returns (bytes memory);
- // Check if a key exists in a json string
- function keyExists(string memory json, string memory key) external returns (bytes memory);
- // Get list of keys in a json string
- function parseJsonKeys(string memory json, string memory key) external returns (string[] memory);
+## Tool used
- // Snapshot the current state of the evm.
- // Returns the id of the snapshot that was created.
- // To revert a snapshot use `revertTo`
- function snapshot() external returns (uint256);
- // Revert the state of the evm to a previous snapshot
- // Takes the snapshot id to revert to.
- // This deletes the snapshot and all snapshots taken after the given snapshot id.
- function revertTo(uint256) external returns (bool);
+Manual Review
- // Creates a new fork with the given endpoint and block,
- // and returns the identifier of the fork
- function createFork(string calldata, uint256) external returns (uint256);
- // Creates a new fork with the given endpoint and the _latest_ block,
- // and returns the identifier of the fork
- function createFork(string calldata) external returns (uint256);
+## Recommendation
+Implement a system where fees are gradually distributed to LP providers.
- // Creates _and_ also selects a new fork with the given endpoint and block,
- // and returns the identifier of the fork
- function createSelectFork(string calldata, uint256)
- external
- returns (uint256);
- // Creates _and_ also selects a new fork with the given endpoint and the
- // latest block and returns the identifier of the fork
- function createSelectFork(string calldata) external returns (uint256);
- // Takes a fork identifier created by `createFork` and
- // sets the corresponding forked state as active.
- function selectFork(uint256) external;
- // Returns the currently active fork
- // Reverts if no fork is currently active
- function activeFork() external returns (uint256);
+## Discussion
- // Updates the currently active fork to given block number
- // This is similar to `roll` but for the currently active fork
- function rollFork(uint256) external;
- // Updates the given fork to given block number
- function rollFork(uint256 forkId, uint256 blockNumber) external;
+**KupiaSecAdmin**
- // Fetches the given transaction from the active fork and executes it on the current state
- function transact(bytes32) external;
- // Fetches the given transaction from the given fork and executes it on the current state
- function transact(uint256, bytes32) external;
+Escalate
- // Marks that the account(s) should use persistent storage across
- // fork swaps in a multifork setup, meaning, changes made to the state
- // of this account will be kept when switching forks
- function makePersistent(address) external;
- function makePersistent(address, address) external;
- function makePersistent(address, address, address) external;
- function makePersistent(address[] calldata) external;
- // Revokes persistent status from the address, previously added via `makePersistent`
- function revokePersistent(address) external;
- function revokePersistent(address[] calldata) external;
- // Returns true if the account is marked as persistent
- function isPersistent(address) external returns (bool);
+This is invalid, `FEES.update` function is called after every action, so when the liquidity of flashloan is added, the accrued fees are distributed to prev LP providers.
- /// Returns the RPC url for the given alias
- function rpcUrl(string calldata) external returns (string memory);
- /// Returns all rpc urls and their aliases `[alias, url][]`
- function rpcUrls() external returns (string[2][] memory);
-}
+**sherlock-admin3**
-```
-
+> Escalate
+>
+> This is invalid, `FEES.update` function is called after every action, so when the liquidity of flashloan is added, the accrued fees are distributed to prev LP providers.
-
- IFee.sol
+You've created a valid escalation!
-```solidity
-// SPDX-License-Identifier: MIT
-pragma solidity >=0.8.13;
+To remove the escalation from consideration: Delete your comment.
-interface IFee {
- function update() external;
- function calc(bool, uint256, uint256) external returns(SumFees memory);
- function calc2(bool, uint256, uint256) external returns(SumFees memory);
- function calc3(bool, uint256, uint256) external returns(SumFees memory);
-
- struct SumFees{
- uint256 funding_paid;
- uint256 funding_received;
- }
-}
-```
-
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
-## Tool used
+**spacegliderrrr**
-Manual Review
+Comment above is simply incorrect. `Fees.update` doesn't actually make a change on the LP token value - it simply calculates the new fee terms and makes a snapshot at current block.
-## Recommendation
-Consider replacing each `fs. * new_terms` with an intermediate calculation using these two values. For a quadratic approximation, include
+The value of LP token is based on the pool reserves. Fees are actually distributed to the pool only upon closing the position, hence why the attack described is possible.
-```vyper
-@internal
-@pure
-def o2(r: uint256, n: uint256) -> uint256:
- if(n == 0):
- return 0
- return r*n + ((n-1)*n*(r**2)/2)/DENOM
+**rickkk137**
+
+invalid
+it is intended design
+```python
+def f(mv: uint256, pv: uint256, ts: uint256) -> uint256:
+ if ts == 0: return mv
+ else : return (mv * ts) / pv
```
+```python
+def g(lp: uint256, ts: uint256, pv: uint256) -> uint256:
+ return (lp * pv) / ts
+```
+Pool contract uses above formola to compute amount of LP token to mint and also burn value for example:
+pool VEL-STX:
+VEL reserve = 1000
+STX reserve = 1000
+VEL/STX price = $1
+LP total_supply = 1000
+**pv stand for pool value and ts stand for total_supply and mv stand for mint value**
+LP_price = ts / pv = 1000 / 1000 = $1
+its mean if user deposit $1000 into pool in result get 1000 LP token
+and for burn
+burn_value = lp_amount * pool_value / total_supply and based on above example
+total_supply=2000
+pool_value=2000
+because user deposit $1000 into pool and mint 1000lp token
-while for a cubic approximation, include
+the issue want to say mailicious user with increase lp price can retrieve borrowing_fee ,but when mailicious users and other users closes their profitable positions pool_value will be decreased[the protocol pay users' profit from pool reserve],hence burn_value become less than usual state
-```vyper
-@internal
-@pure
-def o3(r: uint256, n: uint256) -> uint256:
- if(n == 0):
- return 0
- if(n == 1):
- return r*n
-
- return r*n + ((n-1)*n*(r**2)/2)/DENOM + ((n-2)*(n-1)*n*(r**3)/6)/DENOM**2
-```
+requirment internal state for attack path:
-Refer to `Fee.vy` above for guidance on necessary adjustments to the various functions.
+- mailicious user's position be in loss[pool value will be decreased because user collateral will be added to pool reserve]
+- other user don't close their profitable positions in that block
+- flashloan's cost be low
-The quadratic approximation will provide the largest improvement for the least added computational cost, and is the recommended compromise. As a reference, the quadratic approximation drops the current error from 28.26% to 5.62% (an 80% reduction) given a 4-year timespan and `r = 10`. The cubic approximation further decreases the error to 0.85% (a 97% reduction).
-The error can be made arbitrarily small, at the expense of increased computational costs for diminishing returns. For example, the quartic approximation drops the error down to 0.11% (a 99.6% reduction).
-# Issue M-9: pools.vy: calc_mint does not account for outstanding fees, allowing attackers to steal fees from LP
+**WangSecurity**
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/50
+I see how it can be viewed as the intended design, but still, the user can bypass the fees essentially and LPs lose them, when they should've been to receive it. Am I missing anything here?
-## Found by
-0x37, Trooper
-### Summary
-Inside of the [calc_mint](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L163-L172) the contract calculates how much LP-Tokens should be minted when a provider adds new liquidity. This does not account for outstanding fees. Therefor a new user gets the same LP-Token ratio as older LP providers. **This allows new providers to steal fees.**
+**rickkk137**
-### Root Cause
+attack path is possible when user's position is in lose and loss amount has to be enough to increase the LP price , note that if loss amount be large enough the position can be eligible for liquidation and the user has to close his position before liquidation bot
-In the [`calc_mint`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L163-L172) function, LP tokens are calculated based on the [`total_reserves`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L119-L121) , which reflect the current pool state but do not account for any pending fees. This allows a large token holder to deposit just before a regular user closes their position.
+**spacegliderrrr**
-When closing a position, the core contract calls [`close`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L258-L276) function, updating the pool reserves to reflect the added tokens paid as fee. As a result, the pool now contains more liquidity.
+@WangSecurity Your comment is correct. Anytime the user is about to close their position (whether it be unprofitable, neutral or slightly profitable), they can perform the attack to avoid paying the borrowing fees, essentially stealing them from the LPs.
-The attacker can then unstake their LP tokens and, based on the ratio of their LP share relative to the total LP value before the attack, they are able to claim a significant portion of the fees paid by the user.
+**WangSecurity**
-### Internal pre-conditions
+> attack path is possible when user's position is in lose and loss amount has to be enough to increase the LP price , note that if loss amount be large enough the position can be eligible for liquidation and the user has to close his position before liquidation bot
-1. Large position will be closed and attacker notices before the tx is settled (i.e. by looking at the mem-pool)
+So these are the constraints:
+1. Loss should be relatively large so it increases the LP price.
+2. The attacker has to perform the attack before the liquidation bot liquidates their position which is also complicated by the private mempool on Bob, so we cannot see the transaction of the liquidation bot.
-### External pre-conditions
+Are the above points correct and is there anything missing?
-N/A
+**deadrosesxyz**
-### Attack Path
+No, loss amount does not need to be large. Attack can be performed on any non-profitable position, so the user avoids paying fees.
-1. Mint large LP position
-2. wait for closing transaction to settle
-3. Burn the LP position
+**WangSecurity**
-### Impact
+In that case, I agree that it's an issue that the user can indeed bypass the fees and prevent the LPs from receiving it. Also, I don't see the pre-condition of the position being non-profitable as an extensive limitation. Moreover, since it can be any non-profitable position, then there is also no requirement for executing the attack before the liquidation bot (unless the position can be liquidated as soon as it's non-profitable).
-The attacker is able to steal fees that should have been paid to the older LP.
+Thus, I see that it should be high severity. If something is missing or these limitations are extensive in the context of the protocol, please let me know. Planning to reject the escalation, but increase severity to high.
-### PoC
+**WangSecurity**
-## Walkthrough
+Result:
+High
+Has duplicates
-A user opens a large position in the pool:
-```vyper
- # user opens positions and closes it after 1 weeks
- tx = open(VEL, STX, True, d(999), 10, price=d(5), sender=long)
- assert not tx.failed
+**sherlock-admin4**
- chain.pending_timestamp += round(datetime.timedelta(weeks=1).total_seconds())
-```
+Escalations have been resolved successfully!
-The attacker now mints new LP tokens with a relatively large position vs. the existing LPs. In this case the new LP is ~100x the old LP:
-```vyper
- mint(VEL, STX, LP, d(10_000_000), d(50_000_000), price=d(5), sender=lp_provider2)
-```
+Escalation status:
+- [KupiaSecAdmin](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/94/#issuecomment-2344292890): rejected
-Close tx settles and fees get paid to the pool:
-```vyper
- tx = close(VEL, STX, 1, price=d(5), sender=long)
+# Issue M-1: LPs will withdraw more value than deposited during pegged token de-peg events
+
+Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/52
+
+## Found by
+4gontuk, KupiaSec
+### Summary
+The [`CONTEXT` function in `gl-sherlock/contracts/api.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/api.vy#L52-L71) uses the `/USD` price for valuation, assuming a 1:1 peg between the quote token and USD. This assumption can fail during de-peg events, leading to incorrect valuations and potential exploitation.
+
+### Root Cause
+The `CONTEXT` function calls the `price` function from the `oracle` contract to get the price of the quote token. This price is adjusted based on the `quote_decimals`, implying it is using the `/USD` price for valuation.
+
+#### Detailed Breakdown
+
+1. **`CONTEXT` Function in `api.vy`**:
+ The `CONTEXT` function calls the [`price` function from the `oracle` contract](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/oracle.vy#L57-L77) to get the price of the quote token.
+
+```python
+def CONTEXT(
+ base_token : address,
+ quote_token: address,
+ desired : uint256,
+ slippage : uint256,
+ payload : Bytes[224]
+) -> Ctx:
+ base_decimals : uint256 = convert(ERC20Plus(base_token).decimals(), uint256)
+ quote_decimals: uint256 = convert(ERC20Plus(quote_token).decimals(), uint256)
+ # this will revert on error
+ price : uint256 = self.ORACLE.price(quote_decimals,
+ desired,
+ slippage,
+ payload)
+ return Ctx({
+ price : price,
+ base_decimals : base_decimals,
+ quote_decimals: quote_decimals,
+ })
```
-The attacker now burns all his LP tokens:
-```vyper
- lp_amount_lp1 = LP.balanceOf(lp_provider)
- burn(VEL, STX, LP, lp_amount_lp2, price=d(5), sender=lp_provider2)
+
+2. **`price` Function in `oracle.vy`**:
+ The `price` function in [`oracle.vy` uses the `extract_price` function](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/oracle.vy#L83-L102) to get the price from the oracle.
+
+```python
+
+########################################################################
+TIMESTAMP: public(uint256)
+
+@internal
+def extract_price(
+ quote_decimals: uint256,
+ payload : Bytes[224]
+) -> uint256:
+ price: uint256 = 0
+ ts : uint256 = 0
+ (price, ts) = self.EXTRACTOR.extractPrice(self.FEED_ID, payload)
+
+ # Redstone allows prices ~10 seconds old, discourage replay attacks
+ assert ts >= self.TIMESTAMP, "ERR_ORACLE"
+ self.TIMESTAMP = ts
+
+ # price is quote per unit base, convert to same precision as quote
+ pd : uint256 = self.DECIMALS
+ qd : uint256 = quote_decimals
+ s : bool = pd >= qd
+ n : uint256 = pd - qd if s else qd - pd
+ m : uint256 = 10 ** n
+ p : uint256 = price / m if s else price * m
+ return p
+
+########################################################################
+PRICES: HashMap[uint256, uint256]
+
+@internal
+def get_or_set_block_price(current: uint256) -> uint256:
+ """
+ The first transaction in each block will set the price for that block.
+ """
+ block_price: uint256 = self.PRICES[block.number]
+ if block_price == 0:
+ self.PRICES[block.number] = current
+ return current
+ else:
+ return block_price
+
+########################################################################
+@internal
+@pure
+def check_slippage(current: uint256, desired: uint256, slippage: uint256) -> bool:
+ if current > desired: return (current - desired) <= slippage
+ else : return (desired - current) <= slippage
+
+@internal
+@pure
+def check_price(price: uint256) -> bool:
+ return price > 0
+
+# eof
+
```
-The original LP only earned 1 of each token:
-```vyper
- lp1_inital_value = d(10_000) * 5 + d(50_000)
- lp1_value = lp1_base * 5 + lp1_quote # (as quote tokens)
- lp1_profit = lp1_value - lp1_inital_value # (as quote tokens)
- assert lp1_profit < 10 # lp1_profit is 6: 1 of base and quote
+
+3. **`extract_price` Function in `oracle.vy`**:
+ The `extract_price` function adjusts the price based on the `quote_decimals`, which implies it is using the `/USD` price for valuation.
+
+```83:102:gl-sherlock/contracts/oracle.vy
+def extract_price(
+ quote_decimals: uint256,
+ payload : Bytes[224]
+) -> uint256:
+ price: uint256 = 0
+ ts : uint256 = 0
+ (price, ts) = self.EXTRACTOR.extractPrice(self.FEED_ID, payload)
+
+ # Redstone allows prices ~10 seconds old, discourage replay attacks
+ assert ts >= self.TIMESTAMP, "ERR_ORACLE"
+ self.TIMESTAMP = ts
+
+ # price is quote per unit base, convert to same precision as quote
+ pd : uint256 = self.DECIMALS
+ qd : uint256 = quote_decimals
+ s : bool = pd >= qd
+ n : uint256 = pd - qd if s else qd - pd
+ m : uint256 = 10 ** n
+ p : uint256 = price / m if s else price * m
+ return p
```
-The attacker earned most of the fees, ~120 quote tokens:
-```vyper
- lp2_inital_value = d(10_000_000) * 5 + d(50_000_000)
- lp2_value = lp2_base * 5 + lp2_quote # (as quote tokens)
- lp2_profit = lp2_value - lp2_inital_value # (as quote tokens)
- assert lp2_profit > 100 # lp2_profit ~ 120 (as quote tokens)
+
+### Impact
+During a de-peg event, LPs can withdraw more value than they deposited, causing significant losses to the protocol.
+
+### Attack Path
+1. **Deposit**: Attacker deposits 1 BTC and 50,000 USDT when 1 BTC = 50,000 USD.
+2. **De-peg Event**: The pegged token (USDT) de-pegs to 0.70 USD.
+3. **Withdraw**: Attacker withdraws their funds, exploiting the incorrect assumption that 1 USDT = 1 USD.
+
+### Proof of Concept (PoC)
+1. **Deposit**:
+
+```python
+@external
+def mint(
+ base_token : address, #ERC20
+ quote_token : address, #ERC20
+ lp_token : address, #ERC20Plus
+ base_amt : uint256,
+ quote_amt : uint256,
+ desired : uint256,
+ slippage : uint256,
+ payload : Bytes[224]
+) -> uint256:
+ """
+ @notice Provide liquidity to the pool
+ @param base_token Token representing the base coin of the pool (e.g. BTC)
+ @param quote_token Token representing the quote coin of the pool (e.g. USDT)
+ @param lp_token Token representing shares of the pool's liquidity
+ @param base_amt Number of base tokens to provide
+ @param quote_amt Number of quote tokens to provide
+ @param desired Price to provide liquidity at (unit price using onchain
+ representation for quote_token, e.g. 1.50$ would be
+ 1500000 for USDT with 6 decimals)
+ @param slippage Acceptable deviaton of oracle price from desired price
+ (same units as desired e.g. to allow 5 cents of slippage,
+ send 50000).
+ @param payload Signed Redstone oracle payload
+ """
+ ctx: Ctx = self.CONTEXT(base_token, quote_token, desired, slippage, payload)
+ return self.CORE.mint(1, base_token, quote_token, lp_token, base_amt, quote_amt, ctx)
```
-## Diff
-Update the [`conftest.py`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/tests/conftest.py) to be the same as the currently planed parameters (based on sponsor msg in discord):
-```diff
---- a/conftest.py.orig
-+++ b/conftest.py
-@@ -58,8 +58,8 @@ def LP2(project, core, owner):
- # ----------- Params -----------------
-
- PARAMS = {
-- 'MIN_FEE' : 1,
-- 'MAX_FEE' : 1,
-+ 'MIN_FEE' : 10,
-+ 'MAX_FEE' : 100,
-
- # Fraction of collateral (e.g. 1000).
- 'PROTOCOL_FEE' : 1000,
+2. **De-peg Event**: The pegged token de-pegs to 0.70 USD (external event).
+
+3. **Withdraw**:
+
+```python
+def burn(
+ base_token : address,
+ quote_token : address,
+ lp_token : address,
+ lp_amt : uint256,
+ desired : uint256,
+ slippage : uint256,
+ payload : Bytes[224]
+) -> Tokens:
+ """
+ @notice Withdraw liquidity from the pool
+ @param base_token Token representing the base coin of the pool (e.g. BTC)
+ @param quote_token Token representing the quote coin of the pool (e.g. USDT)
+ @param lp_token Token representing shares of the pool's liquidity
+ @param lp_amt Number of LP tokens to burn
+ @param desired Price to provide liquidity at (unit price using onchain
+ representation for quote_token, e.g. 1.50$ would be
+ 1500000 for USDT with 6 decimals)
+ @param slippage Acceptable deviaton of oracle price from desired price
+ (same units as desired e.g. to allow 5 cents of slippage,
+ send 50000).
+ @param payload Signed Redstone oracle payload
+ """
+ ctx: Ctx = self.CONTEXT(base_token, quote_token, desired, slippage, payload)
+ return self.CORE.burn(1, base_token, quote_token, lp_token, lp_amt, ctx)
```
-```diff
---- a/test_positions.py.orig
-+++ b/test_positions.py
-@@ -137,3 +137,52 @@ def test_value(setup, positions, open, VEL, STX, owner, long):
- 'borrowing_paid_want' : 0,
- 'remaining' : 1998000
- }
-+
-+def test_burn_fees(chain, setup, open, close, VEL, STX, LP, long, mint_token, core, mint, burn, lp_provider, lp_provider2):
-+ setup()
-+
-+ # user opens positions and closes it after 1 weeks
-+ tx = open(VEL, STX, True, d(999), 10, price=d(5), sender=long)
-+ assert not tx.failed
-+
-+ chain.pending_timestamp += round(datetime.timedelta(weeks=1).total_seconds())
-+
-+ # ============= frontrun part
-+ mint_token(VEL, d(10_000_000), lp_provider2)
-+ mint_token(STX, d(50_000_000), lp_provider2)
-+ assert not VEL.approve(core.address, d(10_000_000), sender=lp_provider2).failed
-+ assert not STX.approve(core.address, d(50_000_000), sender=lp_provider2).failed
-+ mint(VEL, STX, LP, d(10_000_000), d(50_000_000), price=d(5), sender=lp_provider2)
-+ # ============= frontrun part
-+
-+ # ============= user tx
-+ tx = close(VEL, STX, 1, price=d(5), sender=long)
-+ assert not tx.failed
-+ # ============= user tx
-+
-+ # ============= backrun part
-+ lp_amount_lp2 = LP.balanceOf(lp_provider2)
-+ burn(VEL, STX, LP, lp_amount_lp2, price=d(5), sender=lp_provider2)
-+ # ============= backrun part
-+
-+ # ============= original lp unstake
-+ lp_amount_lp1 = LP.balanceOf(lp_provider)
-+ burn(VEL, STX, LP, lp_amount_lp1, price=d(5), sender=lp_provider)
-+ # ============= original lp unstake
-+
-+ lp1_base = VEL.balanceOf(lp_provider) - d(90_000) # ignore the unstaked balance
-+ lp1_quote = STX.balanceOf(lp_provider) - d(50_000) # ignore the unstaked balance
-+ lp2_base = VEL.balanceOf(lp_provider2)
-+ lp2_quote = STX.balanceOf(lp_provider2)
-+
-+ # allow for small profit
-+ lp1_inital_value = d(10_000) * 5 + d(50_000)
-+ lp1_value = lp1_base * 5 + lp1_quote # (as quote tokens)
-+ lp1_profit = lp1_value - lp1_inital_value # (as quote tokens)
-+ assert lp1_profit < 10 # lp1_profit is 6: 1 of base and quote
-+
-+ # LP2 made a profit
-+ lp2_inital_value = d(10_000_000) * 5 + d(50_000_000)
-+ lp2_value = lp2_base * 5 + lp2_quote # (as quote tokens)
-+ lp2_profit = lp2_value - lp2_inital_value # (as quote tokens)
-+ assert lp2_profit > 100 # lp2_profit ~ 120 (as quote tokens)
+
+4. **Incorrect Price Calculation**:
+
+```python
+def CONTEXT(
+ base_token : address,
+ quote_token: address,
+ desired : uint256,
+ slippage : uint256,
+ payload : Bytes[224]
+) -> Ctx:
+ base_decimals : uint256 = convert(ERC20Plus(base_token).decimals(), uint256)
+ quote_decimals: uint256 = convert(ERC20Plus(quote_token).decimals(), uint256)
+ # this will revert on error
+ price : uint256 = self.ORACLE.price(quote_decimals,
+ desired,
+ slippage,
+ payload)
+ return Ctx({
+ price : price,
+ base_decimals : base_decimals,
+ quote_decimals: quote_decimals,
+ })
```
+### Mitigation
+To mitigate this issue, the protocol should use the `/` price directly if available, or derive it from the `/USD` and `/USD` prices. This ensures accurate valuations even if the quote token de-pegs from USD.
-### Mitigation
+## Discussion
-The contract `pools.vy` should call the `fees.vy` contract and look up the currently outstanding fees inside of the [`total_reserves`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/pools.vy#L119-L121) function.
+**mePopye**
-# Issue M-10: Fee Precision Loss Disrupts Liquidations and Causes Loss of Funds
+Escalate
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/66
+On behalf of the watson
+
+**sherlock-admin3**
+
+> Escalate
+>
+> On behalf of the watson
+
+You've created a valid escalation!
+
+To remove the escalation from consideration: Delete your comment.
+
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
+
+**WangSecurity**
+
+After additionally considering this issue, here's my understanding. Let's assume a scenario of 30% depeg and USDT = 0.7 USD.
+1. The pool has 10 BTC and 500k USDT.
+2. User deposits 1 BTC and 50k USDT, assuming 1 BTC = 50k USDT = 50k USD.
+3. USDT depegs to 0.7 USD, i.e. 1 USDT = 0.7 USD. Then BTC = 50k USD = ~71.5k USDT.
+4. The user withdraws 1 BTC and 50k USDT. They receive it because the code still considers 1 USDT = 1 USD. There are 10 BTC and 500k USDT left in the contracts.
+5. The protocol didn't lose any funds, the amount of BTC and USDT remained the same as it was before the depeg.
+6. But, in reality, the user has withdrawn 50k worth of BTC and 35k worth of USDT since 1 USDT = 0.7 USD.
+7. Hence, if the protocol accounted for the depeg, there had to be 10 BTC and 515k USDT left in the contract after the user had withdrawn during the depeg.
+
+Hence, even though it's not a direct loss of funds but a loss in value, this should be a valid medium (considering depeg as an extensive limitation). Thus, planning to accept the escalation and validate with medium severity. The duplicate is #113, are there any additional duplicates?
+
+**WangSecurity**
+
+Result:
+Medium
+Has duplicates
+
+**sherlock-admin2**
+
+Escalations have been resolved successfully!
+
+Escalation status:
+- [mePopye](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/52/#issuecomment-2345299625): accepted
+
+# Issue M-2: Funding fee will be zero because of precision loss
+
+Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/72
## Found by
-0xbranded
-## Summary
-Borrowing and funding fees of both longs/shorts suffer from two distinct sources of precision loss. The level of precision loss is large enough to consistently occur at a significant level, and can even result in total omission of fee payment for periods of time. This error is especially disruptive given the sensitive nature of funding fee calculations both in determining liquidations (a core functionality), as well as payments received by LPs and funding recipients (representing a significant loss).
+aslanbek, pashap9990
+### Summary
-## Vulnerability Detail
-The first of the aforementioned sources of precision loss is relating to the `DENOM` parameter defined and used in `apply` of [`math.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/math.vy#L163-L172):
+funding fee in some cases will be zero because of precision loss
-```vyper
-DENOM: constant(uint256) = 1_000_000_000
+### Root Cause
+`long_utilization = base_interest / (base_reserve / 100)`
+`short_utilization = quote_interest / (quote_reserve / 100)`
-def apply(x: uint256, numerator: uint256) -> Fee:
- fee : uint256 = (x * numerator) / DENOM
-...
- return Fee({x: x, fee: fee_, remaining: remaining})
-```
+`borrow_long_fee = max_fee * long_utilization` [min = 10]
+`borrow_short_fee = max_fee * short_utilization` [min = 10]
-This function is in turn referenced (to extract the `fee` parameter in particular) in several locations throughout [`fees.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/fees.vy#L265), namely in determining the funding and borrowing payments made by positions open for a duration of time:
+`funding_fee_long = borrow_long_fee * (long_utilization - short_utilization) / 100`
-```vyper
- paid_long_term : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
-...
- paid_short_term : uint256 = self.apply(fs.short_collateral, fs.funding_short * new_terms)
-...
- P_b : uint256 = self.apply(collateral, period.borrowing_long) if long else (
- self.apply(collateral, period.borrowing_short) )
- P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
- self.apply(collateral, period.funding_short) )
-```
+`funding_fee_short = borrow_short_fee * (short_utilization - long_utilization) / 100`
-The comments for `DENOM` specify that it's a "magic number which depends on the smallest fee one wants to support and the blocktime." In fact, given its current value of $10^{-9}$, the smallest representable fee per block is $10^{-7}$%. Given the average blocktimes of 2.0 sec on the [BOB chain](https://explorer.gobob.xyz/), there are 15_778_800 blocks in a standard calendar year. Combined with the fee per block, this translates to an annual fee of 1.578%.
+let's assume alice open a long position with min collateral[5e6] and leverage 2x when btc/usdt $50,000
-However, this is also the interval size for annualized fees under the current system. As a result, any fee falling below the next interval will be rounded down. For example, given an annualized funding rate in the neighborhood of 15%, there is potential for a nearly 10% error in the interest rate if rounding occurs just before the next interval. This error is magnified the smaller the funding rates become. An annual fee of 3.1% would round down to 1.578%, representing an error of nearly 50%. And any annualized fees below 1.578% will not be recorded, representing a 100% error.
+long_utilization = 0.0002e8 / (1000e8 / 100) = 2e4 / 1e9 = 0.00002[round down => 0]
-The second source of precision loss combines with the aforementioned error, to both increase the severity and frequency of error. It's related to how percentages are handled in `params.vy`, particularly when the long/short utilization is calculated to determine funding & borrow rates. The [utilization](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L59-L63) is shown below:
+short_utilization = 0 / 1e12 /100 = 0
-```vyper
-def utilization(reserves: uint256, interest: uint256) -> uint256:
- return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
-```
+borrowing_long_fee = 100 * (0) = 0 [min fee = 1] ==> 10
+borrowing_short_fee = 100 * (0) = 0 [min fee = 1] ==> 10
-This function is in turn used to calculate borrowing (and funding rates, following a slightly different approach that similarly combines the use of `utilization` and `scale`), in `[dynamic_fees](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L33-L55)` of `params.vy`:
+funding_fee_long = 10 * (0) = 0
+funding_fee_short = 10 * 0 = 0
-```vyper
-def dynamic_fees(pool: PoolState) -> DynFees:
- long_utilization : uint256 = self.utilization(pool.base_reserves, pool.base_interest)
- short_utilization: uint256 = self.utilization(pool.quote_reserves, pool.quote_interest)
- borrowing_long : uint256 = self.check_fee(
- self.scale(self.PARAMS.MAX_FEE, long_utilization))
- borrowing_short : uint256 = self.check_fee(
- self.scale(self.PARAMS.MAX_FEE, short_utilization))
-...
-def scale(fee: uint256, utilization: uint256) -> uint256:
- return (fee * utilization) / 100
+1000 block passed
+
+funding_paid = 5e6 * 1000 * 0 / 1_000_000_000 = 0
+borrowing_paid = (5e6) * (1000 * 10) / 1_000_000_000 = 50
+
+
+** long_utilization and short_utilization are zero until **base_reserve / 100 >= base_interest** and **quote_reserve / 100 >= quote_interest**
+
+
+### Internal pre-conditions
+pool status:
+ "base_reserve" : 1000e8 BTC
+ "quote_reserve" : 1,000,000e6 USDT
+```typescript
+"collector" : "0xCFb56482D0A6546d17535d09f571F567189e88b3",
+ "symbol" : "WBTCUSDT",
+ "base_token" : "0x03c7054bcb39f7b2e5b2c7acb37583e32d70cfa3",
+ "quote_token" : "0x05d032ac25d322df992303dca074ee7392c117b9",
+ "base_decimals" : 8,
+ "quote_decimals": 6,
+ "blocktime_secs": 3,
+ "parameters" : {
+ "MIN_FEE" : 1,
+ "MAX_FEE" : 100,
+ "PROTOCOL_FEE" : 1000,
+ "LIQUIDATION_FEE" : 2,
+
+ "MIN_LONG_COLLATERAL" : 5000000,
+ "MAX_LONG_COLLATERAL" : 100000000000,
+ "MIN_SHORT_COLLATERAL" : 10000,
+ "MAX_SHORT_COLLATERAL" : 200000000,
+
+ "MIN_LONG_LEVERAGE" : 1,
+ "MAX_LONG_LEVERAGE" : 10,
+ "MIN_SHORT_LEVERAGE" : 1,
+ "MAX_SHORT_LEVERAGE" : 10,
+
+ "LIQUIDATION_THRESHOLD": 5
+ },
+ "oracle": {
+ "extractor": "0x3DaF1A3ABF9dd86ee0f7Dd13a256400d01866E04",
+ "feed_id" : "BTC",
+ "decimals" : 8
+ }
```
-Note that `interest` and `reserves` maintain the same precision. Therefore, the output of `utilization` will have just 2 digits of precision, resulting from the division of `reserves` by `100`. However, this approach can similarly lead to fee rates losing a full percentage point in their absolute value. Since the utilization is used by `dynamic_fees` to calculate the funding / borrow rates, when combined with the formerly described source of precision loss the error is greatly amplified.
-Consider a scenario when the long open interest is 199_999 * $10^{18}$ and the reserves are 10_000_000 * $10^{18}$. Under the current `utilization` functionality, the result would be a 1.9999% utilization rounded down to 1%. Further assuming the value of `max_fee = 65` (this represents a 100% annual rate and 0.19% 8-hour rate), the long borrow rate would round down to 0%. Had the 1.9999% utilization rate not been rounded down 1%, the result would have been `r = 1.3`. In turn, the precision loss in `DENOM` would have effectively rounded down to `r = 1`, resulting in a 2.051% borrow rate rounded down to 1.578%.
+### Code Snippet
-In other words, the precision loss in `DENOM` alone would have resulted in a 23% error in this case. But when combined with the precision loss in percentage points represented in `utilization`, a 100% error resulted. While the utilization and resulting interest rates will typically not be low enough to produce such drastic errors, this hopefully illustrates the synergistic combined impact of both sources of precision loss. Even at higher, more representative values for these rates (such as `r = 10`), errors in fee calculations exceeding 10% will consistently occur.
+https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L63
-## Impact
-All fees in the system will consistently be underpaid by a significant margin, across all pools and positions. Additionally trust/confidence in the system will be eroded as fee application will be unpredictable, with sharp discontinuities in rates even given moderate changes in pool utilization. Finally, positions will be subsidized at the expense of LPs, since the underpayment of fees will make liquidations less likely and take longer to occur. As a result, LPs and funding recipients will have lesser incentive to provide liquidity, as they are consistently being underpaid while taking on a greater counterparty risk.
-As an example, consider the scenario where the long open interest is 1_099_999 * $10^{18}$ and the reserves are 10_000_000 * $10^{18}$. Under the current `utilization` functionality, the result would be a 10.9999% utilization rounded down to 10%. Assuming `max_fee = 65` (100% annually, 0.19% 8-hour), the long borrow rate would be `r = 6.5` rounded down to `r = 6`. A 9.5% annual rate results, whereas the accurate result if neither precision loss occurred is `r = 7.15` or 11.3% annually. The resulting error in the borrow rate is 16%.
-Assuming a long collateral of 100_000 * $10^{18}$, LPs would earn 9_500 * $10^{18}$, when they should earn 11_300 * $10^{18}$, a shortfall of 1_800 * $10^{18}$ from longs alone. Additional borrow fee shortfalls would occur for shorts, in addition to shortfalls in funding payments received.
+### Impact
-Liquidation from borrow rates along should have taken 106 months based on the expected result of 11.3% per annum. However, under the 9.5% annual rate it would take 127 months to liquidate a position. This represents a 20% delay in liquidation time from borrow rates alone, not including the further delay caused by potential underpaid funding rates.
+Funding fee always is lower than what it really should be
-When PnL is further taken into account, these delays could mark the difference between a period of volatility wiping out a position. As a result, these losing positions could run for far longer than should otherwise occur and could even turn into winners. Not only are LP funds locked for longer as a result, they are at a greater risk of losing capital to their counterparty. On top of this, they are also not paid their rightful share of profits, losing out on funds to take on an unfavorably elevated risk.
+### PoC
-Thus, not only do consistent, material losses (significant fee underpayment) occur but a critical, core functionality malfunctions (liquidations are delayed).
+Place below test in tests/test_positions.py and run with `pytest -k test_precision_loss -s`
+```python
+def test_precision_loss(setup, open,VEL, STX, long, positions, pools):
+ setup()
+ open(VEL, STX, True, d(5), 10, price=d(50000), sender=long)
+ chain.mine(1000)
+ fee = positions.calc_fees(1)
+ assert fee.funding_paid == 0
+```
-## Code Snippet
-In the included PoC, three distinct tests demonstrate the individual sources of precision loss, as well as their combined effect. Similar scenarios were demonstrated as discussed above, for example interest = 199_999 * $10^{18} with reserves = 10_000_000 * $10^{18}$ with a max fee of 65.
+### Mitigation
-The smart contracts were stripped to isolate the relevant logic, and [foundry](https://github.com/0xKitsune/Foundry-Vyper) was used for testing. To run the test, clone the repo and place `Denom.vy` in vyper_contracts, and place `Denom.t.sol`, `Cheat.sol`, and `IDenom.sol` under src/test.
+1-scale up long_utilzation and short_utilzation
+2-set min value for long_utilzation and short_utilzation
-
-Denom.vy
-```vyper
-struct DynFees:
- funding_long : uint256
- funding_short : uint256
+## Discussion
-struct PoolState:
- base_collateral : uint256
- quote_collateral : uint256
+**sherlock-admin3**
-struct FeeState:
- t1 : uint256
- funding_long : uint256
- funding_short : uint256
- long_collateral : uint256
- short_collateral : uint256
- funding_long_sum : uint256
- funding_short_sum : uint256
- received_long_sum : uint256
- received_short_sum : uint256
+> Escalate
+>
+> Let’s assume
+> interest = 1_099_999e18
+> reserve=10_000_000e18
+> max_fee = 100
+> long_utilization = interest / reserve / 100 = 1_099_999e18 / 10_000_000e18 / 100 = 10.99 ~ 10 //round down
+> borrowing_fee = max_fee * long_utillization / 100 = 100 * 10.99 / 100 = 10.99
+> after one year
+>
+>
+> //result without precision loss
+> block_per_year = 15_778_800
+> funding_fee_sum = block_per_year * funding_fee = 15778800 * 10.99 = 173,409,012
+> borrowing_long_sum = block_per_year * borrowing_fee = 15778800 * 10.99 = 173,409,012
+>
+> borrowing_paid = collateral * borrowing_long_sum / DENOM = 1_099_999e18 * 173,409,012 / 1e9 = 190,749e18
+>
+> funding_paid = collateral * funding_fee_sum / DENOM = 190,749e18
+>
+> //result with precision loss
+> block_per_year = 15_778_800
+> funding_fee_sum = block_per_year * funding_fee = 15778800 * 10 = 157788000
+> borrowing_long_sum = block_per_year * borrowing_fee = 15778800 * 10 = 157788000
+>
+> borrowing_paid = collateral * borrowing_long_sum / DENOM = 1_099_999e18 * 157788000 / 1e9 = 173,566e18
+>
+> funding_paid = collateral * funding_fee_sum / DENOM = 173,566e18
+>
+> result:1% difference exists in result
+>
+>
+>
+>
+>
+>
-struct SumFees:
- funding_paid : uint256
- funding_received: uint256
+You've created a valid escalation!
+
+To remove the escalation from consideration: Delete your comment.
+
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
-struct Period:
- funding_long : uint256
- funding_short : uint256
- received_long : uint256
- received_short : uint256
+**WangSecurity**
-#starting point hardcoded
-@external
-def __init__():
- self.FEE_STORE = FeeState({
- t1 : block.number,
- funding_long : 1,
- funding_short : 0,
- long_collateral : 10_000_000_000_000_000_000_000_000,
- short_collateral : 10_000_000_000_000_000_000_000_000,
- funding_long_sum : 0,
- funding_short_sum : 0,
- received_long_sum : 0,
- received_short_sum : 0,
- })
+To clarify, due to this precision loss, there will be a 1% loss of the funding fee or am I missing something?
- self.FEE_STORE_AT[block.number] = self.FEE_STORE
+**rickkk137**
- self.FEE_STORE2 = FeeState({
- t1 : block.number,
- funding_long : 1_999,
- funding_short : 0,
- long_collateral : 10_000_000_000_000_000_000_000_000,
- short_collateral : 10_000_000_000_000_000_000_000_000,
- funding_long_sum : 0,
- funding_short_sum : 0,
- received_long_sum : 0,
- received_short_sum : 0,
- })
- self.FEE_STORE_AT2[block.number] = self.FEE_STORE2
-# hardcoded funding rates for the scenario where funding is positive
-@internal
-@view
-def dynamic_fees() -> DynFees:
- return DynFees({
- funding_long : 10,
- funding_short : 0,
- })
-# #hardcoded pool to have 1e24 of quote and base collateral
-@internal
-@view
-def lookup() -> PoolState:
- return PoolState({
- base_collateral : 10_000_000_000_000_000_000_000_000,
- quote_collateral : 10_000_000_000_000_000_000_000_000,
- })
+open_interest = 1,099,999e6
+reserve = 10,000,000e6
+long_util = open_interest / reserve / 100 = 10.99
+borrowing_fee = max_fee * long_util / 100 = 100 * 10.99 / 100 = 10.99
+funding_fee = borrowing_fee * long_util / 100 = 10.99 * 10.99 / 100 = 1.20
-FEE_STORE : FeeState
-FEE_STORE_AT : HashMap[uint256, FeeState]
-FEE_STORE2 : FeeState
-FEE_STORE_AT2 : HashMap[uint256, FeeState]
+lets assume there is a long position with 10000e6 colleral and user want to close his position after a year[15778800 blocks per year]
-@internal
-@view
-def lookupFees() -> FeeState:
- return self.FEE_STORE
+**result without precision loss**
-@internal
-@view
-def lookupFees2() -> FeeState:
- return self.FEE_STORE2
-@internal
-@view
-def fees_at_block(height: uint256) -> FeeState:
- return self.FEE_STORE_AT[height]
+borrowing_paid = collateral * borrowing_sum / DENOM
-@internal
-@view
-def fees_at_block2(height: uint256) -> FeeState:
- return self.FEE_STORE_AT2[height]
+borrowing_paid = 10,000e6 * 15778800 * 10.99 / 1e9 = 1,734,090,120[its mean user has to pay $1734 as borrowing fee]
+funding_paid = collateral * funding_sum / DENOM = 10,000e6 * 1.20 * 15778800 / 1e9 = 189,345,600[its mean user has to pay $189 as funding fee]
-@external
-def update():
- fs: FeeState = self.current_fees()
- fs2: FeeState = self.current_fees2()
+**result with precision loss**
- self.FEE_STORE_AT[block.number] = fs
- self.FEE_STORE = fs
+borrowing_paid = collateral * borrowing_sum / DENOM
+borrowing_paid = 10,000e6 * 15778800 * 10 / 1e9 = 1,577,880,000[its mean user has to pay $1,577 as borrowing fee]
+funding_paid = collateral * funding_sum / DENOM = 10,000e6 * 1 * 15778800 / 1e9 = 157,788,000[its mean user has to pay $157 as funding fee]
- self.FEE_STORE_AT2[block.number] = fs2
- self.FEE_STORE2 = fs2
-#math
-ZEROS: constant(uint256) = 1000000000000000000000000000
-DENOM: constant(uint256) = 1_000_000_000
-DENOM2: constant(uint256) = 1_000_000_000_000
+LPs loss = $157[~1%]
+user pay $32 less than expected [32 * 100 / 189 ~ 16%]
-@internal
-@pure
-def extend(X: uint256, x_m: uint256, m: uint256) -> uint256:
- return X + (m*x_m)
-@internal
-@pure
-def apply(x: uint256, numerator: uint256) -> uint256:
- """
- Fees are represented as numerator only, with the denominator defined
- here. This computes x*fee capped at x.
- """
- fee : uint256 = (x * numerator) / DENOM
- fee_ : uint256 = fee if fee <= x else x
- return fee_
-@internal
-@pure
-def apply2(x: uint256, numerator: uint256) -> uint256:
- """
- Fees are represented as numerator only, with the denominator defined
- here. This computes x*fee capped at x.
- """
- fee : uint256 = (x * numerator) / DENOM2
- fee_ : uint256 = fee if fee <= x else x
- return fee_
-@internal
-@pure
-def divide(paid: uint256, collateral: uint256) -> uint256:
- if collateral == 0: return 0
- else : return (paid * ZEROS) / collateral
-@internal
-@pure
-def multiply(ci: uint256, terms: uint256) -> uint256:
- return (ci * terms) / ZEROS
-@internal
-@pure
-def slice(y_i: uint256, y_j: uint256) -> uint256:
- return y_j - y_i
-@external
-@pure
-def utilization(reserves: uint256, interest: uint256) -> uint256:
- """
- Reserve utilization in percent (rounded down). @audit this is actually rounded up...
- """
- return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
-@external
-@pure
-def utilization2(reserves: uint256, interest: uint256) -> uint256:
- """
- Reserve utilization in percent (rounded down). @audit this is actually rounded up...
- """
- return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100_000))
-@external
-@pure
-def scale(fee: uint256, utilization: uint256) -> uint256:
- return (fee * utilization) / 100
+**rickkk137**
-@external
-@pure
-def scale2(fee: uint256, utilization: uint256) -> uint256:
- return (fee * utilization) / 100_000
+#60 dup of this issue
-@internal
-@view
-def current_fees() -> FeeState:
- """
- Update incremental fee state, called whenever the pool state changes.
- """
- # prev/last updated state
- fs : FeeState = self.lookupFees()
- # current state
- ps : PoolState = self.lookup()
- new_fees : DynFees = self.dynamic_fees()
- # number of blocks elapsed
- new_terms: uint256 = block.number - fs.t1
- funding_long_sum : uint256 = self.extend(fs.funding_long_sum, fs.funding_long, new_terms)
- funding_short_sum : uint256 = self.extend(fs.funding_short_sum, fs.funding_short, new_terms)
+**WangSecurity**
- paid_long_term : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
- received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
+I agree that this issue is correct and indeed identifies the precision loss showcasing the 1% loss. Planning to accept the escalation and validate with medium severity.
- paid_short_term : uint256 = self.apply(fs.short_collateral, fs.funding_short * new_terms)
- received_long_term : uint256 = self.divide(paid_short_term, fs.long_collateral)
+**WangSecurity**
- received_long_sum : uint256 = self.extend(fs.received_long_sum, received_long_term, 1)
- received_short_sum : uint256 = self.extend(fs.received_short_sum, received_short_term, 1)
+Result:
+Medium
+Has duplicates
- if new_terms == 0:
- return FeeState({
- t1 : fs.t1,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : fs.funding_long_sum,
- funding_short_sum : fs.funding_short_sum,
- received_long_sum : fs.received_long_sum,
- received_short_sum : fs.received_short_sum,
- })
- else:
- return FeeState({
- t1 : block.number,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : funding_long_sum,
- funding_short_sum : funding_short_sum,
- received_long_sum : received_long_sum,
- received_short_sum : received_short_sum,
- })
+**sherlock-admin3**
-@internal
-@view
-def current_fees2() -> FeeState:
- """
- Update incremental fee state, called whenever the pool state changes.
- """
- # prev/last updated state
- fs : FeeState = self.lookupFees2()
- # current state
- ps : PoolState = self.lookup()
- new_fees : DynFees = self.dynamic_fees()
- # number of blocks elapsed
- new_terms: uint256 = block.number - fs.t1
+Escalations have been resolved successfully!
- funding_long_sum : uint256 = self.extend(fs.funding_long_sum, fs.funding_long, new_terms)
- funding_short_sum : uint256 = self.extend(fs.funding_short_sum, fs.funding_short, new_terms)
+Escalation status:
+- [rickkk137](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/72/#issuecomment-2348564086): accepted
- paid_long_term : uint256 = self.apply2(fs.long_collateral, fs.funding_long * new_terms)
- received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
+# Issue M-3: LPs cannot specify min amount received in burn function, causing loss of fund for them
- paid_short_term : uint256 = self.apply2(fs.short_collateral, fs.funding_short * new_terms)
- received_long_term : uint256 = self.divide(paid_short_term, fs.long_collateral)
+Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/74
- received_long_sum : uint256 = self.extend(fs.received_long_sum, received_long_term, 1)
- received_short_sum : uint256 = self.extend(fs.received_short_sum, received_short_term, 1)
+## Found by
+PASCAL, pashap9990
+### Summary
+LPs cannot set minimum base or quote amounts when burning LP tokens, leading to potential losses due to price fluctuations during transactions.
- if new_terms == 0:
- return FeeState({
- t1 : fs.t1,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : fs.funding_long_sum,
- funding_short_sum : fs.funding_short_sum,
- received_long_sum : fs.received_long_sum,
- received_short_sum : fs.received_short_sum,
- })
- else:
- return FeeState({
- t1 : block.number,
- funding_long : new_fees.funding_long,
- funding_short : new_fees.funding_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- funding_long_sum : funding_long_sum,
- funding_short_sum : funding_short_sum,
- received_long_sum : received_long_sum,
- received_short_sum : received_short_sum,
- })
+### Root Cause
+LPs cannot set the base received amount and the quote received amount
-@internal
-@view
-def query(opened_at: uint256) -> Period:
- """
- Return the total fees due from block `opened_at` to the current block.
- """
- fees_i : FeeState = self.fees_at_block(opened_at)
- fees_j : FeeState = self.current_fees()
- return Period({
- funding_long : self.slice(fees_i.funding_long_sum, fees_j.funding_long_sum),
- funding_short : self.slice(fees_i.funding_short_sum, fees_j.funding_short_sum),
- received_long : self.slice(fees_i.received_long_sum, fees_j.received_long_sum),
- received_short : self.slice(fees_i.received_short_sum, fees_j.received_short_sum),
- })
+### Impact
+LPs may receive significantly lower amounts than expected when burning LP tokens, resulting in financial losses
-@external
-@view
-def calc(long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
- period: Period = self.query(opened_at)
- P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
- self.apply(collateral, period.funding_short) )
- R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
- self.multiply(collateral, period.received_short) )
+### Code Snippet
+https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/api.vy#L104
+### Internal pre-conditions
+Consider change config in tests/conftest.py
+I do it for better understanding,Fee doesn't important in this issue
+```python
+PARAMS = {
+ 'MIN_FEE' : 0,
+ 'MAX_FEE' : 0,
+
+ 'PROTOCOL_FEE' : 1000
+ ...
+}
+```
+### PoC
+**Textual PoC:**
+we assume protocol fee is zero in this example
+1-Bob mints 20,000e6 LP token[base_reserve:10,000e6, quote_reserve:10,000e6]
+2-Alice opens long position[collateral 1000 STX, LEV:5,Price:1]
+3-Price goes up til $2
+4-Bob calls calc_burn[lp_amt:10,000e6,total_supply:20,000e6][return value:base 3750 VEL,quote 7500 STX]
+5-Bob calls burn with above parameters
+6-Alice calls close position
+7-Alice's tx executed before Bob's tx
+8-Bob's tx will be executed and Bob gets 3875 VEL and 4750 STX
+9-Bob losts $2500
+**Coded PoC:**
+place this test in tests/test_positions.py and run this command `pytest -k test_lost_assets -s`
+```python
+
+def test_lost_assets(setup, VEL, STX, lp_provider, LP, pools, math, open, long, close, burn):
+ setup()
+ #Alice opens position
+ open(VEL, STX, True, d(1000), 5, price=d(1), sender=long)
+
+ reserve = pools.total_reserves(1)
+ assert reserve.base == 10000e6
+ assert reserve.quote == 10000e6
+ #Bob calls calc_burn, Bob's assets in term of dollar is $15,000
+ amts = pools.calc_burn(1, d(10000) , d(20000), ctx(d(2)))
+
+ assert amts.base == 3752500000
+ assert amts.quote == 7495000000
- return SumFees({funding_paid: P_f, funding_received: R_f})
+ #Alice closes her position
+ bef = VEL.balanceOf(long)
+ close(VEL, STX, 1, price=d(2), sender=long)
+ after = VEL.balanceOf(long)
-@internal
-@view
-def query2(opened_at: uint256) -> Period:
- """
- Return the total fees due from block `opened_at` to the current block.
- """
- fees_i : FeeState = self.fees_at_block2(opened_at)
- fees_j : FeeState = self.current_fees2()
- return Period({
- funding_long : self.slice(fees_i.funding_long_sum, fees_j.funding_long_sum),
- funding_short : self.slice(fees_i.funding_short_sum, fees_j.funding_short_sum),
- received_long : self.slice(fees_i.received_long_sum, fees_j.received_long_sum),
- received_short : self.slice(fees_i.received_short_sum, fees_j.received_short_sum),
- })
+ vel_bef = VEL.balanceOf(lp_provider)
+ stx_bef = STX.balanceOf(lp_provider)
-@external
-@view
-def calc2(long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
- period: Period = self.query2(opened_at)
- P_f : uint256 = self.apply2(collateral, period.funding_long) if long else (
- self.apply2(collateral, period.funding_short) )
- R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
- self.multiply(collateral, period.received_short) )
+ amts = pools.calc_burn(1, d(10000) , d(20000), ctx(d(2)))
+ assert amts.base == 3877375030
+ assert amts.quote == 4747749964
+ #Bob's tx will be executed
+ burn(VEL, STX, LP, d(10000), price=d(2), sender=lp_provider)
- return SumFees({funding_paid: P_f, funding_received: R_f})
+ vel_after = VEL.balanceOf(lp_provider)
+ stx_after = STX.balanceOf(lp_provider)
+
+
+ print("vel_diff:", (vel_after - vel_bef) / 1e6)#3877.37503 VEL
+ print("stx_diff:", (stx_after - stx_bef) / 1e6)#4747.749964 STX
+ #received values in term of dollar is ~ $12,500,Bob lost ~ $2500
```
-
+### Mitigation
-
-Denom.t.sol
+Consider adding min_base_amount and min_quote_amount to the burn function's params or adding min_assets_value for example when the price is $2 LPs set this param to $14800, its mean received value worse has to be greater than $14800
-```solidity
-// SPDX-License-Identifier: MIT
-pragma solidity >=0.8.13;
-import {CheatCodes} from "./Cheat.sol";
-import "../../lib/ds-test/test.sol";
-import "../../lib/utils/Console.sol";
-import "../../lib/utils/VyperDeployer.sol";
+## Discussion
-import "../IDenom.sol";
+**rickkk137**
-contract DenomTest is DSTest {
- ///@notice create a new instance of VyperDeployer
- VyperDeployer vyperDeployer = new VyperDeployer();
- CheatCodes vm = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
- IDenom denom;
- uint256 constant YEAR = (60*60*24*(365) + 60*60*24 / 4); //includes leap year
+Escalate
+LP token price directly compute based pool reserve and total supply lp token and the issue clearly states received amount can be less than expected amount and in Coded PoC liquidity provider expected $15000 but in result get $12500
- function setUp() public {
- vm.roll(1);
- ///@notice deploy a new instance of ISimplestore by passing in the address of the deployed Vyper contract
- denom = IDenom(vyperDeployer.deployContract("Denom"));
- }
+loss = $2500[1.6%]
+>Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD
- function testDenom() public {
- uint256 blocksInYr = (YEAR) / 2;
- vm.roll(blocksInYr);
+**sherlock-admin3**
- denom.update();
+> Escalate
+> >Slippage related issues showing a definite loss of funds with a detailed explanation for the same can be considered valid high
- // pools were initialized at block 0
- denom.calc(true, 1e25, 0);
- denom.calc2(true, 1e25, 0);
- }
+You've created a valid escalation!
- function testRounding() public {
- uint max_fee = 100; // set max 8-hour rate to 0.288% (157.8% annually)
+To remove the escalation from consideration: Delete your comment.
- uint256 reserves = 10_000_000 ether;
- uint256 interest1 = 1_000_000 ether;
- uint256 interest2 = 1_099_999 ether;
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
- uint256 util1 = denom.utilization(reserves, interest1);
- uint256 util2 = denom.utilization(reserves, interest2);
+**WangSecurity**
- assertEq(util1, util2); // current calculation yields same utilization for both interests
+However, the LPs can add input slippage parameters, i.e. `desired` and `slippage` to mitigate these issues. Am I missing something here?
- // borrow rate
- uint fee1 = denom.scale(max_fee, util1);
- uint fee2 = denom.scale(max_fee, util2);
+**rickkk137**
- assertEq(fee1, fee2); // results in the same fee also. fee2 should be ~1% higher
- }
+@WangSecurity `desired` and `slippage` just has been used to [control price which fetch from oracle](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/oracle.vy#L122) and protocol uses of that for converting quote to base or base to quote to compute total pool's reserve in terms of quote token but there is 2 tips here
+- burn value = [lp_amt * pool_reserve / total_supply_lp](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/pools.vy#L215C28-L215C29)
+- [pool_reserve will be decreased when users closes their profitable positions](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/positions.vy#L203)
- function testCombined() public {
- // now let's see what would happen if we raised the precision of both fees and percents
- uint max_fee = 65;
- uint max_fee2 = 65_000; // 3 extra digits of precision lowers error by 3 orders of magnitude
+let's examine an example with together:
+u have 1000 lp token and u want convert them to usd and pool_reserve and total_supply_lp is 1000 in our example,
+burn_value = lp_amt * pool_reserve / total_supply = 1000 * 1000 / 1000 = 1000 usd
+based on above value u send your transaction to network but a close profitable transaction will be executed before your transaction and get $100 as payout,its mean pool reserve is 900
+burn_value = lp_amt * pool_reserve / total_supply = 1000 * 900 / 1000 = 900 usd
+u get $900 instead of $1000 and u cannot control this situation as a user
- uint256 reserves = 10_000_000 ether;
- uint256 interest = 199_999 ether; // interest & reserves same in both, only differ in precision.
+**WangSecurity**
- uint256 util1 = denom.utilization(reserves, interest);
- uint256 util2 = denom.utilization2(reserves, interest); // 3 extra digits of precision here also
-
- // borrow rate
- uint fee1 = denom.scale(max_fee, util1);
- uint fee2 = denom.scale2(max_fee2, util2);
+Yeah, I see, thank you. Indeed, the situation which would cause an issue to the user happens after the slippage is checked and this conversion cannot be controlled by the user. Planning to accept the escalation and validate with medium severity. Are there any duplicates (non-escalated; I see there are some escalations about slippage; I will consider them as duplicates if needed)?
- assertEq(fee1 * 1_000, fee2 - 999); // fee 1 is 1.000, fee 2 is 1.999 (~50% error)
- }
-}
+**WangSecurity**
-```
+Result:
+Medium
+Has duplicates
-
+**sherlock-admin4**
-
-Cheat.sol
-```solidity
-interface CheatCodes {
- // This allows us to getRecordedLogs()
- struct Log {
- bytes32[] topics;
- bytes data;
- }
+Escalations have been resolved successfully!
- // Possible caller modes for readCallers()
- enum CallerMode {
- None,
- Broadcast,
- RecurrentBroadcast,
- Prank,
- RecurrentPrank
- }
+Escalation status:
+- [rickkk137](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/74/#issuecomment-2348583408): accepted
- enum AccountAccessKind {
- Call,
- DelegateCall,
- CallCode,
- StaticCall,
- Create,
- SelfDestruct,
- Resume
- }
+# Issue M-4: Invalid Redstone oracle payload size prevents the protocol from working properly
- struct Wallet {
- address addr;
- uint256 publicKeyX;
- uint256 publicKeyY;
- uint256 privateKey;
- }
+Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/75
- struct ChainInfo {
- uint256 forkId;
- uint256 chainId;
- }
+## Found by
+KupiaSec
+## Summary
+In api contract, it uses 224 bytes as maximum length for Redstone's oracle payload, but oracle price data and signatures of 3 signers exceeds 225 bytes thus reverting transactions.
- struct AccountAccess {
- ChainInfo chainInfo;
- AccountAccessKind kind;
- address account;
- address accessor;
- bool initialized;
- uint256 oldBalance;
- uint256 newBalance;
- bytes deployedCode;
- uint256 value;
- bytes data;
- bool reverted;
- StorageAccess[] storageAccesses;
- }
+## Vulnerability Detail
+In every external function of api contract, it uses 224 bytes as maximum size for Redstone oracle payload.
- struct StorageAccess {
- address account;
- bytes32 slot;
- bool isWrite;
- bytes32 previousValue;
- bytes32 newValue;
- bool reverted;
- }
+However, the `RedstoneExtractor` requires oracle data from at least 3 unique signers, as implemented in `PrimaryProdDataServiceConsumerBase` contract. Each signer needs to send token price information like token identifier, price, timestamp, etc and 65 bytes of signature data.
+Just with basic calculation, the oracle payload size exceeds 224 bytes.
- // Derives a private key from the name, labels the account with that name, and returns the wallet
- function createWallet(string calldata) external returns (Wallet memory);
+Here's some proof of how Redstone oracle data is used:
- // Generates a wallet from the private key and returns the wallet
- function createWallet(uint256) external returns (Wallet memory);
+- Check one of transactions from [here](https://dune.com/hatskier/redstone) that uses Redstone oracle.
+- One of transaction is [this one](https://snowtrace.io/tx/0x4a3b8cb8a5287f3da1d0026ea35a968b1f93e2c24bad3e587cef450b364e69ec?chainid=43114) on Avalanche, which has 9571 bytes of data.
+- Check this [Blocksec Explorer](https://app.blocksec.com/explorer/tx/avalanche/0x4a3b8cb8a5287f3da1d0026ea35a968b1f93e2c24bad3e587cef450b364e69ec?line=87), and it also shows the oracle data of 3 signers are passed.
- // Generates a wallet from the private key, labels the account with that name, and returns the wallet
- function createWallet(uint256, string calldata) external returns (Wallet memory);
+As shown from the proof above, the payload size of Redstone data is huge, so setting 224 bytes as upperbound reverts transactions.
- // Signs data, (Wallet, digest) => (v, r, s)
- function sign(Wallet calldata, bytes32) external returns (uint8, bytes32, bytes32);
+## Impact
+Protocol does not work because the payload array size limit is too small.
- // Get nonce for a Wallet
- function getNonce(Wallet calldata) external returns (uint64);
+## Code Snippet
+https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/api.vy#L83
+https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/api.vy#L58
- // Set block.timestamp
- function warp(uint256) external;
+## Tool used
+Manual Review
- // Set block.number
- function roll(uint256) external;
+## Recommendation
+The upperbound size of payload array should be increased to satisfy Redstone oracle payload size.
- // Set block.basefee
- function fee(uint256) external;
- // Set block.difficulty
- // Does not work from the Paris hard fork and onwards, and will revert instead.
- function difficulty(uint256) external;
-
- // Set block.prevrandao
- // Does not work before the Paris hard fork, and will revert instead.
- function prevrandao(bytes32) external;
- // Set block.chainid
- function chainId(uint256) external;
+## Discussion
- // Loads a storage slot from an address
- function load(address account, bytes32 slot) external returns (bytes32);
+**KupiaSecAdmin**
- // Stores a value to an address' storage slot
- function store(address account, bytes32 slot, bytes32 value) external;
+Escalate
- // Signs data
- function sign(uint256 privateKey, bytes32 digest)
- external
- returns (uint8 v, bytes32 r, bytes32 s);
+Information required for this issue to be rejected.
- // Computes address for a given private key
- function addr(uint256 privateKey) external returns (address);
+**sherlock-admin3**
- // Derive a private key from a provided mnemonic string,
- // or mnemonic file path, at the derivation path m/44'/60'/0'/0/{index}.
- function deriveKey(string calldata, uint32) external returns (uint256);
- // Derive a private key from a provided mnemonic string, or mnemonic file path,
- // at the derivation path {path}{index}
- function deriveKey(string calldata, string calldata, uint32) external returns (uint256);
+> Escalate
+>
+> Information required for this issue to be rejected.
- // Gets the nonce of an account
- function getNonce(address account) external returns (uint64);
+You've created a valid escalation!
- // Sets the nonce of an account
- // The new nonce must be higher than the current nonce of the account
- function setNonce(address account, uint64 nonce) external;
+To remove the escalation from consideration: Delete your comment.
- // Performs a foreign function call via terminal
- function ffi(string[] calldata) external returns (bytes memory);
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
- // Set environment variables, (name, value)
- function setEnv(string calldata, string calldata) external;
+**WangSecurity**
- // Read environment variables, (name) => (value)
- function envBool(string calldata) external returns (bool);
- function envUint(string calldata) external returns (uint256);
- function envInt(string calldata) external returns (int256);
- function envAddress(string calldata) external returns (address);
- function envBytes32(string calldata) external returns (bytes32);
- function envString(string calldata) external returns (string memory);
- function envBytes(string calldata) external returns (bytes memory);
+@KupiaSecAdmin, could you help me understand how did you get the 9571 data size? can't see it in the links, but I assume I'm missing it somewhere.
- // Read environment variables as arrays, (name, delim) => (value[])
- function envBool(string calldata, string calldata)
- external
- returns (bool[] memory);
- function envUint(string calldata, string calldata)
- external
- returns (uint256[] memory);
- function envInt(string calldata, string calldata)
- external
- returns (int256[] memory);
- function envAddress(string calldata, string calldata)
- external
- returns (address[] memory);
- function envBytes32(string calldata, string calldata)
- external
- returns (bytes32[] memory);
- function envString(string calldata, string calldata)
- external
- returns (string[] memory);
- function envBytes(string calldata, string calldata)
- external
- returns (bytes[] memory);
+**KupiaSecAdmin**
- // Read environment variables with default value, (name, value) => (value)
- function envOr(string calldata, bool) external returns (bool);
- function envOr(string calldata, uint256) external returns (uint256);
- function envOr(string calldata, int256) external returns (int256);
- function envOr(string calldata, address) external returns (address);
- function envOr(string calldata, bytes32) external returns (bytes32);
- function envOr(string calldata, string calldata) external returns (string memory);
- function envOr(string calldata, bytes calldata) external returns (bytes memory);
-
- // Read environment variables as arrays with default value, (name, value[]) => (value[])
- function envOr(string calldata, string calldata, bool[] calldata) external returns (bool[] memory);
- function envOr(string calldata, string calldata, uint256[] calldata) external returns (uint256[] memory);
- function envOr(string calldata, string calldata, int256[] calldata) external returns (int256[] memory);
- function envOr(string calldata, string calldata, address[] calldata) external returns (address[] memory);
- function envOr(string calldata, string calldata, bytes32[] calldata) external returns (bytes32[] memory);
- function envOr(string calldata, string calldata, string[] calldata) external returns (string[] memory);
- function envOr(string calldata, string calldata, bytes[] calldata) external returns (bytes[] memory);
+@WangSecurity - The data size is fetched from this [transaction](https://snowtrace.io/tx/0x4a3b8cb8a5287f3da1d0026ea35a968b1f93e2c24bad3e587cef450b364e69ec?chainid=43114) which uses RedStone oracle.
+The calldata size of the transaction is 9574 bytes, but the function receives 2 variables which sums up 64 bytes, so remaining bytes `9574 - 64 = 9510` bytes of data is for RedStone oracle payload.
+
+Maybe, it includes some additional oracle data, but the main point is that oracle price data of 3 oracles can't fit in 224 bytes. Because each oracle signature is 65 bytes which means it has 195 bytes of signature, and also the oracle data should include price, timestamp, and token name basically. So it never fits in 224 bytes.
- // Convert Solidity types to strings
- function toString(address) external returns(string memory);
- function toString(bytes calldata) external returns(string memory);
- function toString(bytes32) external returns(string memory);
- function toString(bool) external returns(string memory);
- function toString(uint256) external returns(string memory);
- function toString(int256) external returns(string memory);
+**rickkk137**
- // Sets the *next* call's msg.sender to be the input address
- function prank(address) external;
+i used [minimal foundry](https://github.com/redstone-finance/minimal-foundry-repo) package to generate some payload
+```
+bytes memory redstonePayload0 = getRedstonePayload("BTC:120:8,ETH:69:8");
+//result length 440 chars
+425443000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002cb4178004554480000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000019b45a50001921f3ed12d000000200000022af29790252e02178ff033567c55b4145e48cbec0434729f363a01675d9a88d618583b722a092e3e6d9944b69c79cadf793b2950f1a99fd595933609984ef1c01c0001000000000002ed57011e0000
+ bytes memory redstonePayload = getRedstonePayload("BTC:120:8");
+//result length 312 chars
+425443000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002cb41780001921f3ed1d400000020000001e3cabe40b42498dccea014557946f3bae4d29783c9dc7deb499c5a2d6d1901412cba9924d1c1fdfc429c07361ed9fab2789e191791a31ecdbbd77ab95a373f491c0001000000000002ed57011e0000
+
+def mint(
+ base_token : address, #ERC20
+ quote_token : address, #ERC20
+ lp_token : address, #ERC20Plus
+ base_amt : uint256,
+ quote_amt : uint256,
+ desired : uint256,
+ slippage : uint256,
+ @>>> payload : Bytes[224]
+```
+payload parameter's length is 224 its mean we can pass a string with max length 448 character ,hence we can pass above payload to api functions,I want to say length of payload depend on number of symbols which we pass to get payload and max size for 2 symbol is 440 character
- // Sets all subsequent calls' msg.sender to be the input address
- // until `stopPrank` is called
- function startPrank(address) external;
- // Sets the *next* call's msg.sender to be the input address,
- // and the tx.origin to be the second input
- function prank(address, address) external;
+**KupiaSecAdmin**
- // Sets all subsequent calls' msg.sender to be the input address until
- // `stopPrank` is called, and the tx.origin to be the second input
- function startPrank(address, address) external;
+@rickkk137 - Bytes type is different from strings as documented [here](https://docs.vyperlang.org/en/stable/types.html#byte-arrays), Bytes[224] means 224 bytes.
- // Resets subsequent calls' msg.sender to be `address(this)`
- function stopPrank() external;
+**rickkk137**
- // Reads the current `msg.sender` and `tx.origin` from state and reports if there is any active caller modification
- function readCallers() external returns (CallerMode callerMode, address msgSender, address txOrigin);
+Each pair of hexadecimal digits represents one byte and in above examples first example's length is [440 / 2] 220 bytes and second one's length is [312/2] 156 bytes, further more in [redstoneExtractor contract](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/RedstoneExtractor.sol#L10) developer just uses index 0 which its mean requestRedstonePayload function just get one symbol as a parameter in Velar
- // Sets an address' balance
- function deal(address who, uint256 newBalance) external;
-
- // Sets an address' code
- function etch(address who, bytes calldata code) external;
+**KupiaSecAdmin**
- // Marks a test as skipped. Must be called at the top of the test.
- function skip(bool skip) external;
+@rickkk137 - Seems you misunderstand something here.
+You generated Redstone oracle payload using their example repo that results in 220 bytes, this is the oracle data of one signer. For Verla, it requires oracle data of 3 signers so that it can take median price of 3 prices.
- // Expects an error on next call
- function expectRevert() external;
- function expectRevert(bytes calldata) external;
- function expectRevert(bytes4) external;
+And I agree with the statement that it uses one symbol, yes Verla only requires BTC price from Redstone oracle.
- // Record all storage reads and writes
- function record() external;
+**rickkk137**
- // Gets all accessed reads and write slot from a recording session,
- // for a given address
- function accesses(address)
- external
- returns (bytes32[] memory reads, bytes32[] memory writes);
-
- // Record all account accesses as part of CREATE, CALL or SELFDESTRUCT opcodes in order,
- // along with the context of the calls.
- function startStateDiffRecording() external;
+Data is packed into a message according to the following structure
- // Returns an ordered array of all account accesses from a `startStateDiffRecording` session.
- function stopAndReturnStateDiff() external returns (AccountAccess[] memory accesses);
+>The data signature is verified by checking if the signer is one of the approved providers
- // Record all the transaction logs
- function recordLogs() external;
+its mean just one sign there is in every payload
- // Gets all the recorded logs
- function getRecordedLogs() external returns (Log[] memory);
+| Symbol | Value | Timestamp | Size(n) | Signature|
+| -------- | -------- | -------- | -------- | -------- |
+| 32b | 32b | 32b | 1b | 65b |
- // Prepare an expected log with the signature:
- // (bool checkTopic1, bool checkTopic2, bool checkTopic3, bool checkData).
- //
- // Call this function, then emit an event, then call a function.
- // Internally after the call, we check if logs were emitted in the expected order
- // with the expected topics and data (as specified by the booleans)
- //
- // The second form also checks supplied address against emitting contract.
- function expectEmit(bool, bool, bool, bool) external;
- function expectEmit(bool, bool, bool, bool, address) external;
+max size for 1 symbol: 32 + 32 + 32 + 1 + 65 = 172 bytes
+https://github.com/redstone-finance/redstone-evm-connector
- // Mocks a call to an address, returning specified data.
- //
- // Calldata can either be strict or a partial match, e.g. if you only
- // pass a Solidity selector to the expected calldata, then the entire Solidity
- // function will be mocked.
- function mockCall(address, bytes calldata, bytes calldata) external;
+```solidity
+function getAuthorisedSignerIndex(
+ address signerAddress
+ ) public view virtual override returns (uint8) {
+ if (signerAddress == 0x8BB8F32Df04c8b654987DAaeD53D6B6091e3B774) {
+ return 0;
+ } else if (signerAddress == 0xdEB22f54738d54976C4c0fe5ce6d408E40d88499) {
+ return 1;
+ } else if (signerAddress == 0x51Ce04Be4b3E32572C4Ec9135221d0691Ba7d202) {
+ return 2;
+ } else if (signerAddress == 0xDD682daEC5A90dD295d14DA4b0bec9281017b5bE) {
+ return 3;
+ } else if (signerAddress == 0x71d00abE308806A3bF66cE05CF205186B0059503) {
+ return 4;
+ } else {
+ revert SignerNotAuthorised(signerAddress);
+ }
+ }
+```
- // Reverts a call to an address, returning the specified error
- //
- // Calldata can either be strict or a partial match, e.g. if you only
- // pass a Solidity selector to the expected calldata, then the entire Solidity
- // function will be mocked.
- function mockCallRevert(address where, bytes calldata data, bytes calldata retdata) external;
- // Clears all mocked and reverted mocked calls
- function clearMockedCalls() external;
- // Expect a call to an address with the specified calldata.
- // Calldata can either be strict or a partial match
- function expectCall(address callee, bytes calldata data) external;
- // Expect a call to an address with the specified
- // calldata and message value.
- // Calldata can either be strict or a partial match
- function expectCall(address callee, uint256, bytes calldata data) external;
- // Gets the _creation_ bytecode from an artifact file. Takes in the relative path to the json file
- function getCode(string calldata) external returns (bytes memory);
- // Gets the _deployed_ bytecode from an artifact file. Takes in the relative path to the json file
- function getDeployedCode(string calldata) external returns (bytes memory);
- // Label an address in test traces
- function label(address addr, string calldata label) external;
-
- // Retrieve the label of an address
- function getLabel(address addr) external returns (string memory);
+**WangSecurity**
- // When fuzzing, generate new inputs if conditional not met
- function assume(bool) external;
+@KupiaSecAdmin just a small clarification, the idea that you need 3 signers in payload is based on which documentation?
- // Set block.coinbase (who)
- function coinbase(address) external;
+**KupiaSecAdmin**
- // Using the address that calls the test contract or the address provided
- // as the sender, has the next call (at this call depth only) create a
- // transaction that can later be signed and sent onchain
- function broadcast() external;
- function broadcast(address) external;
+@WangSecurity - It comes from the RedStone implementation that Verla uses: https://github.com/redstone-finance/redstone-oracles-monorepo/blob/2bbf16cbbaa36f7046034dbbd968f3673a0657e8/packages/evm-connector/contracts/data-services/PrimaryProdDataServiceConsumerBase.sol#L12-L14
- // Using the address that calls the test contract or the address provided
- // as the sender, has all subsequent calls (at this call depth only) create
- // transactions that can later be signed and sent onchain
- function startBroadcast() external;
- function startBroadcast(address) external;
- function startBroadcast(uint256 privateKey) external;
+And you know, usually, using one signer data as oracle causes issue because its data can be malicious, that's how the protocol takes 3 signers and take median price among them.
- // Stops collecting onchain transactions
- function stopBroadcast() external;
- // Reads the entire content of file to string, (path) => (data)
- function readFile(string calldata) external returns (string memory);
- // Get the path of the current project root
- function projectRoot() external returns (string memory);
- // Reads next line of file to string, (path) => (line)
- function readLine(string calldata) external returns (string memory);
- // Writes data to file, creating a file if it does not exist, and entirely replacing its contents if it does.
- // (path, data) => ()
- function writeFile(string calldata, string calldata) external;
- // Writes line to file, creating a file if it does not exist.
- // (path, data) => ()
- function writeLine(string calldata, string calldata) external;
- // Closes file for reading, resetting the offset and allowing to read it from beginning with readLine.
- // (path) => ()
- function closeFile(string calldata) external;
- // Removes file. This cheatcode will revert in the following situations, but is not limited to just these cases:
- // - Path points to a directory.
- // - The file doesn't exist.
- // - The user lacks permissions to remove the file.
- // (path) => ()
- function removeFile(string calldata) external;
- // Returns true if the given path points to an existing entity, else returns false
- // (path) => (bool)
- function exists(string calldata) external returns (bool);
- // Returns true if the path exists on disk and is pointing at a regular file, else returns false
- // (path) => (bool)
- function isFile(string calldata) external returns (bool);
- // Returns true if the path exists on disk and is pointing at a directory, else returns false
- // (path) => (bool)
- function isDir(string calldata) external returns (bool);
-
- // Return the value(s) that correspond to 'key'
- function parseJson(string memory json, string memory key) external returns (bytes memory);
- // Return the entire json file
- function parseJson(string memory json) external returns (bytes memory);
- // Check if a key exists in a json string
- function keyExists(string memory json, string memory key) external returns (bytes memory);
- // Get list of keys in a json string
- function parseJsonKeys(string memory json, string memory key) external returns (string[] memory);
+**WangSecurity**
- // Snapshot the current state of the evm.
- // Returns the id of the snapshot that was created.
- // To revert a snapshot use `revertTo`
- function snapshot() external returns (uint256);
- // Revert the state of the evm to a previous snapshot
- // Takes the snapshot id to revert to.
- // This deletes the snapshot and all snapshots taken after the given snapshot id.
- function revertTo(uint256) external returns (bool);
+Unfortunately, I'm still not convinced enough this is actually a valid finding. Firstly, the transaction linked before, which has 9574 bytes size of calldata and uses the RedStone oracle, doesn't have the Redstone's payload as an input parameter as Velar does it.
+Secondly, this transaction is on Avalanche, while Velar will be deployed on Bob.
- // Creates a new fork with the given endpoint and block,
- // and returns the identifier of the fork
- function createFork(string calldata, uint256) external returns (uint256);
- // Creates a new fork with the given endpoint and the _latest_ block,
- // and returns the identifier of the fork
- function createFork(string calldata) external returns (uint256);
+Hence, this is not a sufficient argument that 224 Bytes won't be enough.
+Thirdly, `payload` is used when calling the [`extract_price`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/RedstoneExtractor.sol#L6) function which doesn't even use that payload. Hence, I don't see a sufficient argument for this being a medium, but before making the decision, I'm giving some time to correct my points.
- // Creates _and_ also selects a new fork with the given endpoint and block,
- // and returns the identifier of the fork
- function createSelectFork(string calldata, uint256)
- external
- returns (uint256);
- // Creates _and_ also selects a new fork with the given endpoint and the
- // latest block and returns the identifier of the fork
- function createSelectFork(string calldata) external returns (uint256);
+**rickkk137**
- // Takes a fork identifier created by `createFork` and
- // sets the corresponding forked state as active.
- function selectFork(uint256) external;
+@WangSecurity u can pass n asset to fetchPayload function and that isn't const its mean payload length is flexible which depend on protocol and when we look at [extract_price](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/RedstoneExtractor.sol#L10) which just uses index 0 its mean they are suppose to pass just one symbol to redstone function to get payload and base on [redstone document](https://github.com/redstone-finance/redstone-evm-connector) payload's length just for one symbol is 172 bytes which is less than 224 bytes
- // Returns the currently active fork
- // Reverts if no fork is currently active
- function activeFork() external returns (uint256);
+payload_size = n*(32 + 32) + 32 + 1 + 65
+payload_size_for_one_asset = 1 * (32 + 32) + 32 + 1 + 65 = 172 bytes
+payload_size_for_two_asset = 2 * (32 + 32) + 32 + 1 + 65 = 226 bytes
+payload_size_for_three_asset = 3 * (32 + 32) + 32 + 1 + 65 = 290 bytes
+...
- // Updates the currently active fork to given block number
- // This is similar to `roll` but for the currently active fork
- function rollFork(uint256) external;
- // Updates the given fork to given block number
- function rollFork(uint256 forkId, uint256 blockNumber) external;
- // Fetches the given transaction from the active fork and executes it on the current state
- function transact(bytes32) external;
- // Fetches the given transaction from the given fork and executes it on the current state
- function transact(uint256, bytes32) external;
- // Marks that the account(s) should use persistent storage across
- // fork swaps in a multifork setup, meaning, changes made to the state
- // of this account will be kept when switching forks
- function makePersistent(address) external;
- function makePersistent(address, address) external;
- function makePersistent(address, address, address) external;
- function makePersistent(address[] calldata) external;
- // Revokes persistent status from the address, previously added via `makePersistent`
- function revokePersistent(address) external;
- function revokePersistent(address[] calldata) external;
- // Returns true if the account is marked as persistent
- function isPersistent(address) external returns (bool);
+**KupiaSecAdmin**
- /// Returns the RPC url for the given alias
- function rpcUrl(string calldata) external returns (string memory);
- /// Returns all rpc urls and their aliases `[alias, url][]`
- function rpcUrls() external returns (string[2][] memory);
-}
+@WangSecurity - The 9574 bytes of payload is one example of Redstone payload.
+
+The point is that it's obvious the price data can't fit in 224 bytes. As @rickkk137 mentioned, payload size for one asset of one signer is 172 bytes, but Verla requires oracle data of 3 signers, which will result in >500 bytes.
+
+**rickkk137**
+
+Velar protocol uses version [0.6.1 redstone-evm-connector](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/package.json#L3) and in this version `RedstoneConsumerBase::getUniqueSignersThreshold` returns 1 in this path `/
+@redstone-finance/evm-connector
+/
+contracts
+/
+core
+/
+RedstoneConsumerBase.sol`,hence just one signer is required
+https://www.npmjs.com/package/@redstone-finance/evm-connector/v/0.6.1?activeTab=code
+also when we look at [make file](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/GNUmakefile#L28C22-L28C42) we realized Velar's developers directly copy RedstoneConsumerBase contract without any changes,furthermore usingDataService function get unique signer as a parameter and when they restricted payload to 224 bytes its mean they want to pass 1 as a uniqueSignersCount
+```typescript
+
+ const wrappedContract = WrapperBuilder.wrap(contract).usingDataService({
+ dataServiceId: "redstone-rapid-demo",
+ @>>> uniqueSignersCount: 1,
+ dataFeeds: ["BTC", "ETH", "BNB", "AR", "AVAX", "CELO"],
+ });
```
+https://github.com/redstone-finance/redstone-showroom/blob/0db580be39bdccb9632ee4d8d8c80e4182d8e266/example/getTokensPrices.ts#L22
-
+**KupiaSecAdmin**
-
-IDenom.sol
+@rickkk137 - Check `RedstoneExtractor.sol`, the contract inherits from `PrimaryProdDataServiceConsumerBase` contract which is located in "./vendor/data-services/PrimaryProdDataServiceConsumerBase.sol" that is copied after make, which returns 3 as number of signers.
-```solidity
-// SPDX-License-Identifier: MIT
-pragma solidity >=0.8.13;
+**WangSecurity**
-interface IDenom {
- function update() external;
- function calc(bool, uint256, uint256) external returns(SumFees memory);
- function calc2(bool, uint256, uint256) external returns(SumFees memory);
+As I understand, @KupiaSecAdmin is indeed correct here, and the current payload size won't work when the contracts are deployed on the Live chain. After clarifying with the sponsor, they've said they used 224 only for testnet and will increase it for the live chain, but there's no info about it in README or code comments. Hence, this should be indeed a valid issue. Planning to accept the escalation and validate with Medium severity. Medium severity because there is no loss of funds, and the second definition of High severity excludes contracts not working:
+> Inflicts serious non-material losses (doesn't include contract simply not working).
- function utilization(uint256, uint256) external returns(uint256);
- function utilization2(uint256, uint256) external returns(uint256);
- function scale(uint256, uint256) external returns(uint256);
- function scale2(uint256, uint256) external returns(uint256);
-
- struct SumFees{
- uint256 funding_paid;
- uint256 funding_received;
- }
-}
+Hence, medium severity is appropriate:
+> Breaks core contract functionality, rendering the contract useless or leading to loss of funds.
+
+Are there any duplicates?
+
+**KupiaSecAdmin**
+
+@WangSecurity - Agree with having this as Medium severity. Thanks for your confirmation.
+
+**WangSecurity**
+
+Result:
+Medium
+Unique
+
+**sherlock-admin4**
+
+Escalations have been resolved successfully!
+
+Escalation status:
+- [KupiaSecAdmin](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/75/#issuecomment-2344252146): accepted
+
+# Issue M-5: Not decreasing oracle timestamp validation leads to DoS for protocol users
+
+Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/79
+
+## Found by
+KupiaSec
+## Summary
+The protocol only allows equal or increased timestamp of oracle prices whenever an action happens in the protocol.
+This validation is wrong since it will lead to DoS for users.
+
+## Vulnerability Detail
+The protocol uses RedStone oracle, where token prices are added as a part of calldata of transactions.
+In RedStone oracle, it allows prices from 3 minutes old upto 1 minute in the future, as implemented in `RedstoneDefaultsLib.sol`.
+
+```vyper
+@internal
+def extract_price(
+ quote_decimals: uint256,
+ payload : Bytes[224]
+) -> uint256:
+ price: uint256 = 0
+ ts : uint256 = 0
+ (price, ts) = self.EXTRACTOR.extractPrice(self.FEED_ID, payload)
+
+ # Redstone allows prices ~10 seconds old, discourage replay attacks
+ assert ts >= self.TIMESTAMP, "ERR_ORACLE"
+ self.TIMESTAMP = ts
```
-
+In `oracle.vy`, it extracts the token price from the RedStone payload, which also includes the timestamp of which the prices were generated.
+As shown in the code snippet, the protocol reverts when the timestamp extracted from the calldata is smaller than the stored timestamp, thus forcing timestamps only increase or equal to previous one.
+This means that the users who execute transaction with price 1 minute old gets reverted when there is another transaction executed with price 30 seconds old.
+
+NOTE: The network speed of all around the world is not same, so there can be considerable delays based on the location, api availability, etc.
+
+By abusing this vulnerability, an attacker can regularly make transactions with newest prices which will revert all other transactions with slightly older price data like 10-20 seconds older, can be all reverted.
+
+## Impact
+The vulnerability causes DoS for users who execute transactions with slightly older RedStone oracle data.
+
+## Code Snippet
+https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/oracle.vy#L91-L93
## Tool used
-
Manual Review
## Recommendation
-Consider increasing the precision of `DENOM` by atleast 3 digits, i.e. `DENOM: constant(uint256) = 1_000_000_000_000` instead of `1_000_000_000`. Consider increasing the precision of percentages by 3 digits, i.e. divide / multiply by 100_000 instead of 100.
+It's recommended to remove that non-decreasing timestamp validation.
+If the protocol wants more strict oracle price validation than the RedStone does, it can just use the difference between oracle timestamp and current timestamp.
-Each added digit of precision decreases the precision loss by an order of magnitude. In other words the 1% and 1.5% absolute errors in precision would shrink to 0.01% and 0.015% when using three extra digits of precision.
-Consult `Denom.vy` for further guidance on necessary adjustments to make to the various functions to account for these updated values.
-# Issue M-11: Inequitable Fee Application Penalizes Lower Leverage Positions and LPs
+## Discussion
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/68
+**KupiaSecAdmin**
-## Found by
-0xbranded
-## Summary
-Funding rates are [typically applied](https://www.investopedia.com/what-are-perpetual-futures-7494870) to the [notional value](https://www.investopedia.com/terms/n/notionalvalue.asp) of a perpetual futures contract. However, the application of all fees in the protocol only takes into account the collateral value of the position. This is despite the `interest` of a pool determining both locked `reserves` and the values of dynamic rates.
+Escalate
-Positions with high leverage not only lock more `reserves`, but also increase the funding/borrowing rates more than positions with low leverage. These increased rates apply equally to all positions, penalizing low leverage positions which have to pay disproportionately more fees relative to their `interest` contribution, and face elevated liquidation risk as a result.
+Information required for this issue to be rejected.
-The protocol, and LPs are also penalized by these higher leverage positions, since they will earn less fees while locking the same amount of `reserves`. While the risk of liquidation is higher for these positions, the lower notional fees establish added perverse incentives for taking on high leverage than would otherwise be the case. As a result, positions with low leverage will pay far greater fees and LPs (as well as the protocol) earn less fees than they otherwise would for locking up the same amount of capital.
+**sherlock-admin3**
-## Vulnerability Detail
-When applying borrowing and funding fees to a position, the `calc` function of [`fees.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/fees.vy#L265-L270) is utilized:
+> Escalate
+>
+> Information required for this issue to be rejected.
-```vyper
-def calc(id: uint256, long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
- period: Period = self.query(id, opened_at)
- P_b : uint256 = self.apply(collateral, period.borrowing_long) if long else (
- self.apply(collateral, period.borrowing_short) )
- P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
- self.apply(collateral, period.funding_short) )
-...
-```
+You've created a valid escalation!
-which queries the globally accrued interest during the time the position was active, and applies it to the `collateral` value using the `apply` function of [`math.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/math.vy#L167):
+To remove the escalation from consideration: Delete your comment.
-```vyper
-def apply(x: uint256, numerator: uint256) -> Fee:
- fee : uint256 = (x * numerator) / DENOM
-...
-```
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
-Finally, when opening a position for the first time (in [`core.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/core.vy#L244)) a static fee is also applied directly to the collateral value (from [`params.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L93):
+**rickkk137**
-```vyper
-def open(
-...
- cf : Fee = self.PARAMS.static_fees(collateral0)
- collateral : uint256 = cf.remaining
-...
-def static_fees(collateral: uint256) -> Fee:
- fee : uint256 = collateral / self.PARAMS.PROTOCOL_FEE
- remaining: uint256 = collateral - fee
- return Fee({x: collateral, fee: fee, remaining: remaining})
-```
-
-In effect, all fees are applied directly to the collateral value of positions and ignore the value of the leverage, or corresponding open-interest. As such, a position with higher `collateral` will always pay more fees than one with less `collateral`, even if the former has a lower `interest`.
+the protocol for every action fetch a redstone's payload and attach that to the transaction let's assume:
+User A:fetch payload a in T1
+user B:fetch payload b in T2
+user C:fetch payload c in T3
+and T3 > T2 > T1 and block.timestamp-[T1,T2,T3] < 3 minutes(I mean all of them are valid)
+if all of them send their Txs to the network sequence is very important its mean just with this sequence [t1,t2,t3] none of TXs wouldn't be reverted,
+if t2 will be executed first then t1 will be reverted and if t3 will be executed first t1 and t2 will be reverted
-Positions which take on a higher `leverage` and corresponding`interest` value contribute disproportionately to the [funding and borrowing rates for their side](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L33-L55), by pushing up the `utilization`:
+```diff
+ contract RedstoneExtractor is PrimaryProdDataServiceConsumerBase {
++ uint lastTimeStamp;
++ uint price;
+ function extractPrice(bytes32 feedId, bytes calldata)
+ public view returns(uint256, uint256)
+ {
++ if(block.timestamp - lastTimeStamp > 3 minutes){
+ bytes32[] memory dataFeedIds = new bytes32[](1);
+ dataFeedIds[0] = feedId;
+ (uint256[] memory values, uint256 timestamp) =
+ getOracleNumericValuesAndTimestampFromTxMsg(dataFeedIds);
+ validateTimestamp(timestamp); //!!!
+- return (values[0], timestamp);
++ lastTimeStamp = block.timestamp;
++ price = values[0];
++ }
++ return (price, lastTimeStamp);
+ }
+ }
+ ```
-```vyper
-def dynamic_fees(pool: PoolState) -> DynFees:
- long_utilization : uint256 = self.utilization(pool.base_reserves, pool.base_interest)
- short_utilization: uint256 = self.utilization(pool.quote_reserves, pool.quote_interest)
- borrowing_long : uint256 = self.check_fee(
- self.scale(self.PARAMS.MAX_FEE, long_utilization))
- borrowing_short : uint256 = self.check_fee(
- self.scale(self.PARAMS.MAX_FEE, short_utilization))
- funding_long : uint256 = self.funding_fee(
- borrowing_long, long_utilization, short_utilization)
- funding_short : uint256 = self.funding_fee(
- borrowing_short, short_utilization, long_utilization)
-...
+But there isn't loss of funds ,users can repeat their TXs
-def utilization(reserves: uint256, interest: uint256) -> uint256:
- return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
- })
-```
+**KupiaSecAdmin**
-Note that the fee calculations are also utilized in determining whether a given position is liquidatable in [`positions.vy`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L352):
+@rickkk137 - Thanks for providing the PoC. Of course there's no loss of funds, and users can repeat their transactions but those can be reverted again. Overall, there's pretty high chance that users' transactions will bereverted.
-```vyper
-def is_liquidatable(id: uint256, ctx: Ctx) -> bool:
- v: PositionValue = Positions(self).value(id, ctx)
- return self.PARAMS.is_liquidatable(v.position, v.pnl)
-
-def value(id: uint256, ctx: Ctx) -> PositionValue:
- pos : PositionState = Positions(self).lookup(id)
- fees : FeesPaid = Positions(self).calc_fees(id)
- pnl : PnL = Positions(self).calc_pnl(id, ctx, fees.remaining) # collateral less fees used here
-...
- return PositionValue({position: pos, fees: fees, pnl: pnl, deltas: deltas})
+**rickkk137**
-def is_liquidatable(position: PositionState, pnl: PnL) -> bool:
- percent : uint256 = self.PARAMS.LIQUIDATION_THRESHOLD * position.leverage
- required: uint256 = (position.collateral * percent) / 100
- return not (pnl.remaining > required)
-```
+I agree with u and this can be problematic and Here's an [example](https://github.com/euler-xyz/euler-price-oracle/blob/eeb1847df7d9d58029de37225dabf963bf1a65e6/src/adapter/redstone/RedstoneCoreOracle.sol#L71C9-L72C75) of this approach but the issue's final result depend on sherlock rules
-where the collateral + PnL - (dynamic) fees is compared with some fraction of the original collateral balance. All positions on the same side of a pool face the same dynamic rates, regardless of their level of leverage.
+**WangSecurity**
-## Impact
-Positions with higher than average leverage pay less fees as a fraction of their notional value. Since the same global rates are assessed on all positions of the same side of a pool, high leverage positions are effectively subsidized by lower leverage ones. High leverage positions will also take significantly longer to liquidate than they otherwise would if their contribution to their pool's `interest` was taken into account.
+Not sure I understand how this can happen.
-They also drive up the dynamic rates more, and lock a greater quantity of global reserves than those having less leverage. Since they pay less notional fees than positions with lower leverage, perverse incentives further encourage taking out greater leverage than normal.
+For example, we fetched the price from RedStone at timestamp = 10.
+Then someone fetches the price again, and for the revert to happen, the timestamp of that price has to be 9, correct?
-As a result, lower leverage positions will pay more fees than they otherwise would. They pay a far greater share of the global funding and borrowing fees relative to the amount of `interest` that LPs will potentially need to pay out for their winning positions. They also face increased risk of liquidation since the elevated fees increases the likelihood of a wipeout, as well as reducing the amount of time until absolute fee liquidation.
+How could that happen? Is it the user who chooses the price?
-The high leverage positions on one side of a pool will liquidate from fees at the same rate as lower leverage positions, despite providing far less collateral. If the leverage were taken into account, these positions would liquidate multiple times faster due to fees alone. These positions have high open-interest and contribute disproportionately to the locked reserves. As a result, LPs face greater counterparty risk, especially during trending market conditions where PnL liquidations are less likely. Additionally, they receive less fees which further reduces the risk-reward of providing liquidity.
+**KupiaSecAdmin**
-As a demonstration, consider the following pool with `base_reserves = quote_reserves = 1e7 * 1e18`. It's assumed to have `quote_interest = 4e5 * 1e18` and `base_collateral = 2e5 * 1e18`. For simplicity, it has only two open long positions with the same collateral (but differing leverage):
-1. `quote_collateral = 1e5 * 1e18`, but `base_interest = 1e6 * 1e18` (10x leverage)
-2. `quote_collateral = 1e5 * 1e18`, but `base_interest = 2e5 * 1e18` (2x leverage)
+@WangSecurity - As a real-world example, there will be multiple users who get price from RedStone and call Verla protocol with that price data. NOTE: price data(w/ timestamp) is added as calldata for every call.
+So among those users, there will be users calling protocol with price timestamp 8, some with 9, some with 10.
-Further, assume [`PARAMS.MAX_FEE = 300`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L43). Both positions were opened in the same block, and both closed exactly 1 calendar year later. Pool conditions are assumed to have remained the same.
+If one call with price timestamp 10 is called, other remaining calls with price timestamp 8 and 9 will revert.
-The combined `quote_collateral = 2e5 * 1e18`, and `base_interest = 12e5 * 1e8`. Using the reserves from earlier, this results in a long utilization of 12%, which corresponds to a borrowing rate of `r = 36`, using the `PARAMS.MAX_FEE = 300` from earlier. The short utilization is calculated to be 4%, thus the imbalance in funding rates is 8% for longs, resulting in a funding rate of `r = 2` using the same `PARAMS.MAX_FEE = 300`.
+**WangSecurity**
-Given the [2 second blocktime](https://explorer.gobob.xyz/) on BOB chain, these positions were closed after 15_768_000 blocks, in which time they each accrued an 56.76% borrow fee and 3.15% funding fee - a total fee of 59.9%. Since both positions had `quote_collateral = 1e5 * 1e18`, their quote collateral was discounted by the same amount before calculating PnL: `0.599e5 * 1e18`.
+Thank you for this clarification. Indeed, this is possible, and it can happen even intentionally. On the other hand, this is only one-block DOS and the users could proceed with the transactions in the next block (assuming they use the newer price). However, this vulnerability affects the opening and closing of positions, which are time-sensitive functions. Hence, this should be a valid medium based on this rule:
+> Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.
-Note that both positions paid the same percentage and nominal fee rates. However the first position took up 10% of the base reserves, while the second took up only 2% **yet they paid the exact same fees.**
+Planning to accept the escalation and validate with medium severity.
- To further emphasize this point, let's look at two scenarios where each position has the same leverage, and the total `base_interest` is the same.
-- Keep position 1. For position 2, `quote_collateral = 2e4 * 1e18` and `base_interest = 2e5 * 1e18`
-- Keep position 2. For position 1, `quote_collateral = 5e5 * 1e18` and `base_interest = 1e6 * 1e18`
+**WangSecurity**
-The same borrow / funding rates from before apply since the overall `reserves` and `interest` didn't change. Position 2 now pays a nominal fee of just `0.1198e5 * 1e18`, which is a fifth of that paid before. Position 1 now pays a nominal fee of `2.995e5 * 1e18`, which is five times what was paid before.
+Result:
+Medium
+Unique
-In either case, the same amount of reserves were used. However, when the total pool leverage is 10x, the total fee subtracted from longs was `0.7189e5 * 1e18` tokens, and when it was 2x, the total fee was `3.594e5 * 1e18` tokens. **Five times more fee tokens were paid when the total pool leverage was 5x higher, despite the same level of reserves (counterparty risk) in either case.**
+**sherlock-admin4**
-Significant loss of funds occurs due to inflated fees by lower-leveraged positions, and decreased fee receipts from the protocol and LPs. It will also disrupt the normal liquidation process, as lower-leveraged positions will liquidate at a faster clip than they otherwise would. LP funds are locked for a longer duration than they would be if the fee was assessed fairly on the notional value of these high leverage positions.
+Escalations have been resolved successfully!
-## Code Snippet
-For the PoC, `r = 10`, `long_collateral = short_collateral = 10^7 * 10^18` and `n = 6.3 * 10^7` blocks (4 years), as outlined above.
+Escalation status:
+- [KupiaSecAdmin](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/79/#issuecomment-2344257659): accepted
-The smart contracts were stripped to isolate the relevant logic, and [foundry](https://github.com/0xKitsune/Foundry-Vyper) was used for testing. To run the test, clone the repo and place `Leverage.vy` in `vyper_contracts`, and place `Leverage.t.sol`, `Cheat.sol`, and `ILeverage.sol` under `src/test`.
-
-Leverage.vy
+# Issue M-6: Usage of `tx.origin` to determine the user is prone to attacks
-```vyper
+Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/82
-struct PositionState:
- collateral : uint256
- interest : uint256
+## Found by
+Bauer, Greed, Japy69, KupiaSec, Waydou, bughuntoor, ctf\_sec, y4y
+## Summary
+Usage of `tx.origin` to determine the user is prone to attacks
-struct DynFees:
- borrowing_long : uint256
- borrowing_short: uint256
- funding_long : uint256
- funding_short : uint256
+## Vulnerability Detail
+Within `core.vy` to user on whose behalf it is called is fetched by using `tx.origin`.
+```vyper
+ self._INTERNAL()
-struct PoolState:
- base_reserves : uint256
- quote_reserves : uint256
- base_interest : uint256
- quote_interest : uint256
- base_collateral : uint256
- quote_collateral : uint256
+ user : address = tx.origin
+```
-# need to edit all old stuff returning ps
-# struct PoolState:
-# base_collateral : uint256
-# quote_collateral : uint256
+This is dangerous, as any time a user calls/ interacts with an unverified contract, or a contract which can change implementation, they're put under risk, as the contract can make a call to `api.vy` and act on user's behalf.
-#added borrowing long & borrowing short, update it elsewhere
-struct FeeState:
- t1 : uint256
- funding_long : uint256
- borrowing_long : uint256
- funding_short : uint256
- borrowing_short : uint256
- long_collateral : uint256
- short_collateral : uint256
- borrowing_long_sum : uint256
- borrowing_short_sum : uint256
- funding_long_sum : uint256
- funding_short_sum : uint256
- received_long_sum : uint256
- received_short_sum : uint256
+Usage of `tx.origin` would also break compatibility with Account Abstract wallets.
-struct SumFees:
- total: uint256
- funding_paid : uint256
- funding_received: uint256
- borrowing_paid: uint256
+## Impact
+Any time a user calls any contract on the BOB chain, they risk getting their funds lost.
+Incompatible with AA wallets.
-struct Period:
- borrowing_long : uint256
- borrowing_short: uint256
- funding_long : uint256
- funding_short : uint256
- received_long : uint256
- received_short : uint256
+## Code Snippet
+https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/core.vy#L166
-MAX_FEE: uint256
-MIN_FEE: uint256
+## Tool used
-FEE_STORE: FeeState
-FEE_STORE_AT: HashMap[uint256, FeeState]
+Manual Review
-POOL_STORE: PoolState
+## Recommendation
+Instead of using `tx.origin` in `core.vy`, simply pass `msg.sender` as a parameter from `api.vy`
-POSITION_STORE: PositionState
-POSITION_STORE2: PositionState
-#starting point hardcoded
-@external
-def __init__(_qc: uint256, _bi: uint256, _qc2: uint256, _bi2: uint256):
- self.MAX_FEE = 0
- self.MAX_FEE = 300
-
- self.POOL_STORE = PoolState({
- base_reserves : 10_000_000_000_000_000_000_000_000,
- quote_reserves : 10_000_000_000_000_000_000_000_000,
- base_interest : _bi + _bi2,
- quote_interest : 400_000_000_000_000_000_000_000,
- base_collateral : 200_000_000_000_000_000_000_000,
- quote_collateral : _qc + _qc2,
- })
- starting_fees: DynFees = self.dynamic_fees()
+## Discussion
- self.FEE_STORE = FeeState({
- t1 : 1,
- funding_long : starting_fees.funding_long,
- borrowing_long : starting_fees.borrowing_long,
- funding_short : starting_fees.funding_short,
- borrowing_short : starting_fees.borrowing_short,
- long_collateral : _qc + _qc2,
- short_collateral : 200_000_000_000_000_000_000_000,
- borrowing_long_sum : 0,
- borrowing_short_sum : 0,
- funding_long_sum : 0,
- funding_short_sum : 0,
- received_long_sum : 0,
- received_short_sum : 0,
- })
+**T1MOH593**
- self.FEE_STORE_AT[1] = self.FEE_STORE
+Escalate
- self.POSITION_STORE = PositionState({
- collateral: _qc,
- interest: _bi,
- })
+Noticed there were 19 escalations on preliminary valid issues. This is final escalation to make it 20/20 🙂
- self.POSITION_STORE2 = PositionState({
- collateral: _qc2,
- interest: _bi2,
- })
+**sherlock-admin3**
-@internal
-@view
-def dynamic_fees() -> DynFees:
- pool: PoolState = self.POOL_STORE
+> Escalate
+>
+> Noticed there were 19 escalations on preliminary valid issues. This is final escalation to make it 20/20 🙂
- long_utilization : uint256 = self.utilization(pool.base_reserves, pool.base_interest)
- short_utilization: uint256 = self.utilization(pool.quote_reserves, pool.quote_interest)
- borrowing_long : uint256 = self.check_fee(
- self.scale(self.MAX_FEE, long_utilization))
- borrowing_short : uint256 = self.check_fee(
- self.scale(self.MAX_FEE, short_utilization))
- funding_long : uint256 = self.funding_fee(
- borrowing_long, long_utilization, short_utilization)
- funding_short : uint256 = self.funding_fee(
- borrowing_short, short_utilization, long_utilization)
- return DynFees({
- borrowing_long : borrowing_long,
- borrowing_short: borrowing_short,
- funding_long : funding_long,
- funding_short : funding_short,
- })
+You've created a valid escalation!
-@internal
-@pure
-def utilization(reserves: uint256, interest: uint256) -> uint256:
- return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
+To remove the escalation from consideration: Delete your comment.
-@internal
-@pure
-def scale(fee: uint256, utilization: uint256) -> uint256:
- return (fee * utilization) / 100
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
-@internal
-@view
-def check_fee(fee: uint256) -> uint256:
- if self.MIN_FEE <= fee and fee <= self.MAX_FEE: return fee
- elif fee < self.MIN_FEE : return self.MIN_FEE
- else : return self.MAX_FEE
+**WangSecurity**
-@internal
-@pure
-def imbalance(n: uint256, m: uint256) -> uint256:
- return n - m if n >= m else 0
+bruh
-@internal
-@view
-def funding_fee(base_fee: uint256, col1: uint256, col2: uint256) -> uint256:
- imb: uint256 = self.imbalance(col1, col2)
- if imb == 0: return 0
- else : return self.check_fee(self.scale(base_fee, imb))
+**WangSecurity**
-# #hardcoded pool to have 1e24 of quote and base collateral
-@internal
-@view
-def lookup() -> PoolState:
- return self.POOL_STORE
+Planning to reject the escalation and leave the issue as it is.
-@internal
-@view
-def lookupFees() -> FeeState:
- return self.FEE_STORE
+**WangSecurity**
-@internal
-@view
-def fees_at_block(height: uint256) -> FeeState:
- return self.FEE_STORE_AT[height]
+Result:
+Medium
+Has duplicates
-@external
-def update():
- fs: FeeState = self.current_fees()
+**sherlock-admin4**
- self.FEE_STORE = fs
+Escalations have been resolved successfully!
+Escalation status:
+- [T1MOH593](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/82/#issuecomment-2347091773): rejected
-#math
-ZEROS: constant(uint256) = 1000000000000000000000000000
-DENOM: constant(uint256) = 1_000_000_000
+# Issue M-7: Funding Paid != Funding Received
-@internal
-@pure
-def extend(X: uint256, x_m: uint256, m: uint256) -> uint256:
- return X + (m*x_m)
+Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/83
-@internal
-@pure
-def apply(x: uint256, numerator: uint256) -> uint256:
- """
- Fees are represented as numerator only, with the denominator defined
- here. This computes x*fee capped at x.
- """
- fee : uint256 = (x * numerator) / DENOM
- fee_ : uint256 = fee if fee <= x else x
- return fee_
+## Found by
+0x37, 0xbranded, bughuntoor
+## Summary
+Due to special requirements around receiving funding fees for a position, the funding fees received can be less than that paid. These funding fee payments are still payed, but a portion of them will not be withdrawn, and become stuck funds. This also violates the contract specification that `sum(funding_received) = sum(funding_paid)`.
-@internal
-@pure
-def divide(paid: uint256, collateral: uint256) -> uint256:
- if collateral == 0: return 0
- else : return (paid * ZEROS) / collateral
+## Vulnerability Detail
+In [`calc_fees`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L257-L263) there are two special conditions that impact a position's receipt of funding payments:
-@internal
-@pure
-def multiply(ci: uint256, terms: uint256) -> uint256:
- return (ci * terms) / ZEROS
+```vyper
+ # When there are negative positions (liquidation bot failure):
+ avail : uint256 = pool.base_collateral if pos.long else (
+ pool.quote_collateral )
+ # 1) we penalize negative positions by setting their funding_received to zero
+ funding_received: uint256 = 0 if remaining == 0 else (
+ # 2) funding_received may add up to more than available collateral, and
+ # we will pay funding fees out on a first come first serve basis
+ min(fees.funding_received, avail) )
+```
-@internal
-@pure
-def slice(y_i: uint256, y_j: uint256) -> uint256:
- return y_j - y_i
+If the position has run out of collateral by the time it is being closed, he will receive none of his share of funding payments. Additionally, if the available collateral is not high enough to service the funding fee receipt, he will receive only the greatest amount that is available.
-@internal
-@view
-def current_fees() -> FeeState:
- """
- Update incremental fee state, called whenever the pool state changes.
- """
- # prev/last updated state
- fs : FeeState = self.lookupFees()
- # current state
- ps : PoolState = self.lookup()
- new_fees : DynFees = self.dynamic_fees()
- # number of blocks elapsed
- new_terms: uint256 = block.number - fs.t1
+These funding fee payments are still always made ([deducted](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L250) from remaining collateral), whether they are received or not:
- borrowing_long_sum : uint256 = self.extend(fs.borrowing_long_sum, fs.borrowing_long, new_terms)
- borrowing_short_sum : uint256 = self.extend(fs.borrowing_short_sum, fs.borrowing_short, new_terms)
- funding_long_sum : uint256 = self.extend(fs.funding_long_sum, fs.funding_long, new_terms)
- funding_short_sum : uint256 = self.extend(fs.funding_short_sum, fs.funding_short, new_terms)
+```vyper
+ c1 : Val = self.deduct(c0, fees.funding_paid)
+```
- paid_long_term : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
- received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
+When a position is closed under most circumstances, the pool will have enough collateral to service the corresponding fee payment:
- paid_short_term : uint256 = self.apply(fs.short_collateral, fs.funding_short * new_terms)
- received_long_term : uint256 = self.divide(paid_short_term, fs.long_collateral)
+```vyper
+# longs
+base_collateral : [self.MATH.MINUS(fees.funding_received)],
+quote_collateral: [self.MATH.PLUS(fees.funding_paid),
+ self.MATH.MINUS(pos.collateral)],
+...
+# shorts
+base_collateral : [self.MATH.PLUS(fees.funding_paid), # <-
+ self.MATH.MINUS(pos.collateral)],
+quote_collateral: [self.MATH.MINUS(fees.funding_received)],
+```
- received_long_sum : uint256 = self.extend(fs.received_long_sum, received_long_term, 1)
- received_short_sum : uint256 = self.extend(fs.received_short_sum, received_short_term, 1)
+When positions are closed, the original collateral (which was placed into the pool upon opening) is removed. However, the amount of funding payments a position made is added to the pool for later receipt. Thus, when positions are still open there is enough position collateral to fulfill the funding fee payment and when they close the funding payment made by that position still remains in the pool.
- if new_terms == 0:
- return FeeState({
- t1 : fs.t1,
- funding_long : new_fees.funding_long,
- borrowing_long : new_fees.borrowing_long,
- funding_short : new_fees.funding_short,
- borrowing_short : new_fees.borrowing_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- borrowing_long_sum : fs.borrowing_long_sum,
- borrowing_short_sum : fs.borrowing_short_sum,
- funding_long_sum : fs.funding_long_sum,
- funding_short_sum : fs.funding_short_sum,
- received_long_sum : fs.received_long_sum,
- received_short_sum : fs.received_short_sum,
- })
- else:
- return FeeState({
- t1 : block.number,
- funding_long : new_fees.funding_long,
- borrowing_long : new_fees.borrowing_long,
- funding_short : new_fees.funding_short,
- borrowing_short : new_fees.borrowing_short,
- long_collateral : ps.quote_collateral,
- short_collateral : ps.base_collateral,
- borrowing_long_sum : borrowing_long_sum,
- borrowing_short_sum : borrowing_short_sum,
- funding_long_sum : funding_long_sum,
- funding_short_sum : funding_short_sum,
- received_long_sum : received_long_sum,
- received_short_sum : received_short_sum,
- })
+Only when the amount of funding a position paid exceeded its original collateral, will there will not be enough collateral to service the receipt of funding fees, as alluded to in the comments. However, it's possible for users to pay the full funding fee, but if the borrow fee exceeds the collateral balance remaining thereafter, they will not receive any funding fees. As a result, it's possible for funding fees to be paid which are never received.
+
+Further, even in the case of funding fee underpayment, setting the funding fee received to 0 does not remedy this issue. The funding fees which he underpaid were in a differing token from those which he would receive, so this only furthers the imbalance of fees received to paid.
-@internal
-@view
-def query(opened_at: uint256) -> Period:
- """
- Return the total fees due from block `opened_at` to the current block.
- """
- fees_i : FeeState = self.fees_at_block(opened_at)
- fees_j : FeeState = self.current_fees()
- return Period({
- borrowing_long : self.slice(fees_i.borrowing_long_sum, fees_j.borrowing_long_sum),
- borrowing_short : self.slice(fees_i.borrowing_short_sum, fees_j.borrowing_short_sum),
- funding_long : self.slice(fees_i.funding_long_sum, fees_j.funding_long_sum),
- funding_short : self.slice(fees_i.funding_short_sum, fees_j.funding_short_sum),
- received_long : self.slice(fees_i.received_long_sum, fees_j.received_long_sum),
- received_short : self.slice(fees_i.received_short_sum, fees_j.received_short_sum),
- })
+## Impact
+`core.vy` includes a specification for one of the invariants of the protocol:
+```vyper
+# * funding payments match
+# sum(funding_received) = sum(funding_paid)
+```
-@external
-@view
-def calc() -> SumFees:
- long: bool = True
- collateral: uint256 = self.POSITION_STORE.collateral
- opened_at: uint256 = 1
+This invariant is clearly broken as some portion of paid funding fees will not be received under all circumstances, so code is not to spec. This will also lead to some stuck funds, as a portion of the paid funding fees will never be deducted from the collateral. This can in turn lead to dilution of fees for future funding fee recipients, as the payments will be distributed evenly to the entire collateral including these stuck funds which will never be removed.
- period: Period = self.query(opened_at)
- P_b : uint256 = self.apply(collateral, period.borrowing_long) if long else (
- self.apply(collateral, period.borrowing_short) )
- P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
- self.apply(collateral, period.funding_short) )
- R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
- self.multiply(collateral, period.received_short) )
+## Code Snippet
- return SumFees({total: P_f + P_b - R_f, funding_paid: P_f, funding_received: R_f, borrowing_paid: P_b})
+## Tool used
+Manual Review
-@external
-@view
-def calc2() -> SumFees:
- long: bool = True
- collateral: uint256 = self.POSITION_STORE2.collateral
- opened_at: uint256 = 1
+## Recommendation
+Consider an alternative method of accounting for funding fees, as there are many cases under the current accounting where fees received/paid can fall out of sync.
- period: Period = self.query(opened_at)
- P_b : uint256 = self.apply(collateral, period.borrowing_long) if long else (
- self.apply(collateral, period.borrowing_short) )
- P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
- self.apply(collateral, period.funding_short) )
- R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
- self.multiply(collateral, period.received_short) )
+For example, include a new state variable that explicitly tracks unpaid funding fee payments and perform some pro rata or market adjustment to future funding fee recipients, specifically for *that token*.
- return SumFees({total: P_f + P_b - R_f, funding_paid: P_f, funding_received: R_f, borrowing_paid: P_b})
-```
-
+## Discussion
-
-Leverage.t.sol
+**spacegliderrrr**
-```solidity
-// SPDX-License-Identifier: MIT
-pragma solidity >=0.8.13;
+Escalate
-import {CheatCodes} from "./Cheat.sol";
+Issue should be invalidated. It does not showcase any Medium-severity impact, but rather just a slightly incorrect code comment. Contest readme did not include said invariant as one that must always be true, so simply breaking it slightly does not warrant Medium severity.
-import "../../lib/ds-test/test.sol";
-import "../../lib/utils/Console.sol";
-import "../../lib/utils/VyperDeployer.sol";
+**sherlock-admin3**
-import "../ILeverage.sol";
+> Escalate
+>
+> Issue should be invalidated. It does not showcase any Medium-severity impact, but rather just a slightly incorrect code comment. Contest readme did not include said invariant as one that must always be true, so simply breaking it slightly does not warrant Medium severity.
-contract LeverageTest is DSTest {
- ///@notice create a new instance of VyperDeployer
- VyperDeployer vyperDeployer = new VyperDeployer();
- CheatCodes vm = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
- ILeverage lev;
- uint256 constant YEAR = 60 * 60 * 24 * 365;
- uint256 blocksInYr = (YEAR) / 2;
+You've created a valid escalation!
- function setUp() public {
- vm.roll(1);
- }
+To remove the escalation from consideration: Delete your comment.
- function testDiffLev() public {
- // quote collateral & base interest of positions as laid out in scenario 1
- uint256 qc = 100_000 ether;
- uint256 bi = 1_000_000 ether;
- uint256 qc2 = 100_000 ether;
- uint256 bi2 = 200_000 ether;
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
- lev = ILeverage(vyperDeployer.deployContract("Leverage",
- abi.encode(qc, bi, qc2, bi2)));
+**msheikhattari**
- vm.roll(blocksInYr);
+> If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.
- lev.update();
+The included code comment was an explicit invariant outlined by the team:
+```
+# - the protocol handles the accounting needed to maintain the sytem invariants:
+# * funding payments match
+# sum(funding_received) = sum(funding_paid)
+```
- // pools were initialized at block 0
- lev.calc();
- lev.calc2();
- }
+The problematic code was actually intended to enforce this invariant, however it does so incorrectly. In the case of a negative position due to fee underpayment, `sum(funding_received) > sum(funding_paid)`. To correct for this, or serve as a deterrent, these positions will not receive their funding payment. However the token in which they underpaid is not the same token that they will not receive funding payment for. As a result the imbalance between funding paid and received is not corrected - it is actually worsened.
- function testBoth10x() public {
- // quote collateral & base interest of positions as laid out in scenario 1
- uint256 qc = 100_000 ether;
- uint256 bi = 1_000_000 ether;
- uint256 qc2 = 20_000 ether;
- uint256 bi2 = 200_000 ether;
+Not only that, but users may have paid their full funding payment and the `sum(funding_received) = sum(funding_paid)` invariant holds. But if the remaining balance was then not enough to cover their borrow fee, they will not receive their funding payment which would actually cause this invariant to break.
- lev = ILeverage(vyperDeployer.deployContract("Leverage",
- abi.encode(qc, bi, qc2, bi2)));
+This specification is the source of truth, and the code clearly does not abide by it. The issue proposes alternative methods for accounting funding fee payments to ensure this invariant consistently holds.
- vm.roll(blocksInYr);
+**msheikhattari**
- lev.update();
+Also I do think this issue is similar to #18 and #93
- // pools were initialized at block 0
- lev.calc();
- lev.calc2();
- }
+Will leave it to HoJ if they are combined since the source of the error is the same, but different impacts are described.
- function testBoth2x() public {
- // quote collateral & base interest of positions as laid out in scenario 1
- uint256 qc = 500_000 ether;
- uint256 bi = 1_000_000 ether;
- uint256 qc2 = 100_000 ether;
- uint256 bi2 = 200_000 ether;
+This issue discusses the code being not to spec and breaking an invariant.
+The other two issues mention a known issue from the sponsor of stuck/undistributed funds
- lev = ILeverage(vyperDeployer.deployContract("Leverage",
- abi.encode(qc, bi, qc2, bi2)));
+**WangSecurity**
- vm.roll(blocksInYr);
+@spacegliderrrr is correct that indeed breaking the invariant from the code comment doesn't make the issue medium-severity necessarily. But, as I understand, it also works as intended. The position pays its fees, but if there is not enough collateral, then position cannot pay the fee and this fee will be unpaid/underpaid. And, IIUC, in this protocol the borrowing fees comes first and if the remaining collateral cannot pay the funding fee in full, then it shouldn't be. Hence, this is also enough of a contextual evidence that the comment is outdated. Do I miss anything here?
- lev.update();
+**spacegliderrrr**
- // pools were initialized at block 0
- lev.calc();
- lev.calc2();
- }
-}
+@WangSecurity Most of your points are correct. Yes the code works as expected. Funding fee comes before borrowing fee. But for both of them, since due to fail to liquidate position on time, fees can exceed the total position collateral. Because of this, funding fees are paid with priority and if there's anything left, borrowing fees are paid for as much as there's left.
-```
+In the case where funding fees are overpaid (for more than the total position collateral), the other side receives these fees in a FIFO order, which is also clearly stated in the comments.
-
+**msheikhattari**
-
-Cheat.sol
+@spacegliderrrr is correct, funding fees are paid before borrow fees. As such there is no evidence that the comment spec is outdated, nor does any other documentation such as the readme appear to indicate so.
-```solidity
-interface CheatCodes {
- // This allows us to getRecordedLogs()
- struct Log {
- bytes32[] topics;
- bytes data;
- }
+> In the case where funding fees are overpaid (for more than the total position collateral), the other side receives these fees in a FIFO order, which is also clearly stated in the comments.
- // Possible caller modes for readCallers()
- enum CallerMode {
- None,
- Broadcast,
- RecurrentBroadcast,
- Prank,
- RecurrentPrank
- }
+To elaborate on this point, the vulnerability describes the case that `funding_received` from the other side is greater than the `funding_paid`. While it is indeed acknowledged fees are received in FIFO order, this is a problematic mitigation for this issue. This current approach is a questionable means of correcting the imbalance of funding payments to receipts. There are many cases where it not only doesn't correct for the funding payment imbalance, but actually worsens it, as explained above.
- enum AccountAccessKind {
- Call,
- DelegateCall,
- CallCode,
- StaticCall,
- Create,
- SelfDestruct,
- Resume
- }
+Regardless, this seems to be a clearly defined invariant. Not only from this comment but further implied by the logic of this fee handling, which penalizes negative positions to build up some "insurance" tokens to hedge against the case that funding fees are underpaid. It also intuitively makes sense for this invariant to generally hold as otherwise malfunctions can occur such as failure to close positions; several linked issues reported various related problems stemming from this invariant breaking as well.
- struct Wallet {
- address addr;
- uint256 publicKeyX;
- uint256 publicKeyY;
- uint256 privateKey;
- }
+**WangSecurity**
- struct ChainInfo {
- uint256 forkId;
- uint256 chainId;
- }
+Another question I've got after re-reading the report. The impact section says there will be stuck tokens in the contract which will never be paid. But, as I understand the problem is different. The issue is that these tokens are underpaid, e.g. if the funding fee is 10 tokens, but the remaining collateral is only 8, then there are 2 tokens underpaid. So, how the are stuck tokens in the contract, if the funding fee is not paid in full. Or it refers to the funding_received being larger than the funding_received actually?
- struct AccountAccess {
- ChainInfo chainInfo;
- AccountAccessKind kind;
- address account;
- address accessor;
- bool initialized;
- uint256 oldBalance;
- uint256 newBalance;
- bytes deployedCode;
- uint256 value;
- bytes data;
- bool reverted;
- StorageAccess[] storageAccesses;
- }
+**WangSecurity**
- struct StorageAccess {
- address account;
- bytes32 slot;
- bool isWrite;
- bytes32 previousValue;
- bytes32 newValue;
- bool reverted;
- }
+Since no answer is provided, I assume it's a typo in the report and the contract doesn't have fewer tokens, since the fee is underpaid, not overpaid.
+But, the issue here is not medium severity, because the position with collateral less than the funding fees and it cannot pay the full fee. Just breaking the invariant is not sufficient for medium severity. Planning to accept the escalation and invalidate the issue.
- // Derives a private key from the name, labels the account with that name, and returns the wallet
- function createWallet(string calldata) external returns (Wallet memory);
+**msheikhattari**
- // Generates a wallet from the private key and returns the wallet
- function createWallet(uint256) external returns (Wallet memory);
+Apologies for the delayed response @WangSecurity
- // Generates a wallet from the private key, labels the account with that name, and returns the wallet
- function createWallet(uint256, string calldata) external returns (Wallet memory);
+Yes, the original statement was as intended. That portion of the report was pointing out that there are two problematic impacts of the current approach that cause `sum(funding_paid) != sum(funding_received)`
+1. It's possible for `funding_received` by one side of the pool to exceed the `funding_paid` by the other side, in the case that a position went negative.
+2. It's possible for `funding_received` to be less than `funding_paid`, even for positions which did not go insolvent due to funding rates.
- // Signs data, (Wallet, digest) => (v, r, s)
- function sign(Wallet calldata, bytes32) external returns (uint8, bytes32, bytes32);
+The outlined invariant is broken which results in loss of funds / broken functionality. It will prevent closing of some positions in the former case, and will result in some funds being stuck in the latter case.
- // Get nonce for a Wallet
- function getNonce(Wallet calldata) external returns (uint64);
+The current approach of penalizing negative positions is intended to 'build up an insurance' as stated by the team. But as mentioned here, not only does it not remediate the issue it actually furthers the imbalance. The funding tokens have already been underpaid in the case of a negative position - preventing those positions from receiving their fees results in underpayment of their funding as well.
- // Set block.timestamp
- function warp(uint256) external;
+**WangSecurity**
- // Set block.number
- function roll(uint256) external;
+> It's possible for funding_received by one side of the pool to exceed the funding_paid by the other side, in the case that a position went negative.
- // Set block.basefee
- function fee(uint256) external;
+Could you elaborate on this? Similar to my analysis [here](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/86#issuecomment-2381323660), If the position is negative, but the collateral is not 0, the funding paid is larger than the position's collateral, the funding paid will be decreased to pos. collateral. The funding received will be 0 (if collateral is < funding paid, c1: remaining =0 and deducted =pos.collateral. Then c2: remaining =0 and deducted =0. funding received =0, because remaining =0. So, not sure how funding_received can be > funding paid and I need a more concrete example with numbers.
- // Set block.difficulty
- // Does not work from the Paris hard fork and onwards, and will revert instead.
- function difficulty(uint256) external;
-
- // Set block.prevrandao
- // Does not work before the Paris hard fork, and will revert instead.
- function prevrandao(bytes32) external;
+> It's possible for funding_received to be less than funding_paid, even for positions which did not go insolvent due to funding rates.
- // Set block.chainid
- function chainId(uint256) external;
+Agree that it's possible.
- // Loads a storage slot from an address
- function load(address account, bytes32 slot) external returns (bytes32);
+> The outlined invariant is broken which results in loss of funds / broken functionality. It will prevent closing of some positions in the former case, and will result in some funds being stuck in the latter case.
- // Stores a value to an address' storage slot
- function store(address account, bytes32 slot, bytes32 value) external;
+However, the report doesn't showcase that this broken invariant (when funding received < funding paid) will result in any of this. The report only says that it will lead to a dilution of fees, but there is still no explanation of how.
- // Signs data
- function sign(uint256 privateKey, bytes32 digest)
- external
- returns (uint8 v, bytes32 r, bytes32 s);
+Hence, without a concrete example of how this leads to positions not being closed, thus leading to losses of these positions, and owners, I cannot verify.
- // Computes address for a given private key
- function addr(uint256 privateKey) external returns (address);
+I see that the invariant doesn't hold, but this is not sufficient for Medium severity (only broken invariants from README are assigned Medium, not invariants from code comments). Hence, the decision for now is to invalidate this issue and accept the escalation. But, if there's a POC, how does this lead to loss of funds in case funding received < funding paid, or how does funding received > funding paid and how does this lead to a loss of funds, I will consider changing my decision.
- // Derive a private key from a provided mnemonic string,
- // or mnemonic file path, at the derivation path m/44'/60'/0'/0/{index}.
- function deriveKey(string calldata, uint32) external returns (uint256);
- // Derive a private key from a provided mnemonic string, or mnemonic file path,
- // at the derivation path {path}{index}
- function deriveKey(string calldata, string calldata, uint32) external returns (uint256);
+**WangSecurity**
- // Gets the nonce of an account
- function getNonce(address account) external returns (uint64);
+If no answer is provided, planning to accept the escalation and invalidate the issue.
- // Sets the nonce of an account
- // The new nonce must be higher than the current nonce of the account
- function setNonce(address account, uint64 nonce) external;
+**msheikhattari**
- // Performs a foreign function call via terminal
- function ffi(string[] calldata) external returns (bytes memory);
+> I see that the invariant doesn't hold, but this is not sufficient for Medium severity (only broken invariants from README are assigned Medium, not invariants from code comments)
- // Set environment variables, (name, value)
- function setEnv(string calldata, string calldata) external;
+Doesn't the below rule from the severity categorization apply here? That's what I had in mind when creating the issue, since this comment was the only source of documentation on this specification. There is no indication that this is out of date based on any other specs or code elsewhere.
- // Read environment variables, (name) => (value)
- function envBool(string calldata) external returns (bool);
- function envUint(string calldata) external returns (uint256);
- function envInt(string calldata) external returns (int256);
- function envAddress(string calldata) external returns (address);
- function envBytes32(string calldata) external returns (bytes32);
- function envString(string calldata) external returns (string memory);
- function envBytes(string calldata) external returns (bytes memory);
+```
+Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines).
- // Read environment variables as arrays, (name, delim) => (value[])
- function envBool(string calldata, string calldata)
- external
- returns (bool[] memory);
- function envUint(string calldata, string calldata)
- external
- returns (uint256[] memory);
- function envInt(string calldata, string calldata)
- external
- returns (int256[] memory);
- function envAddress(string calldata, string calldata)
- external
- returns (address[] memory);
- function envBytes32(string calldata, string calldata)
- external
- returns (bytes32[] memory);
- function envString(string calldata, string calldata)
- external
- returns (string[] memory);
- function envBytes(string calldata, string calldata)
- external
- returns (bytes[] memory);
+If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth.
+```
- // Read environment variables with default value, (name, value) => (value)
- function envOr(string calldata, bool) external returns (bool);
- function envOr(string calldata, uint256) external returns (uint256);
- function envOr(string calldata, int256) external returns (int256);
- function envOr(string calldata, address) external returns (address);
- function envOr(string calldata, bytes32) external returns (bytes32);
- function envOr(string calldata, string calldata) external returns (string memory);
- function envOr(string calldata, bytes calldata) external returns (bytes memory);
-
- // Read environment variables as arrays with default value, (name, value[]) => (value[])
- function envOr(string calldata, string calldata, bool[] calldata) external returns (bool[] memory);
- function envOr(string calldata, string calldata, uint256[] calldata) external returns (uint256[] memory);
- function envOr(string calldata, string calldata, int256[] calldata) external returns (int256[] memory);
- function envOr(string calldata, string calldata, address[] calldata) external returns (address[] memory);
- function envOr(string calldata, string calldata, bytes32[] calldata) external returns (bytes32[] memory);
- function envOr(string calldata, string calldata, string[] calldata) external returns (string[] memory);
- function envOr(string calldata, string calldata, bytes[] calldata) external returns (bytes[] memory);
+Nevertheless there are loss of funds from each case, let me first prove that `funding_received > funding_paid` is possible:
- // Convert Solidity types to strings
- function toString(address) external returns(string memory);
- function toString(bytes calldata) external returns(string memory);
- function toString(bytes32) external returns(string memory);
- function toString(bool) external returns(string memory);
- function toString(uint256) external returns(string memory);
- function toString(int256) external returns(string memory);
+> Then c2: remaining =0 and deducted =0. funding received =0, because remaining =0.
- // Sets the *next* call's msg.sender to be the input address
- function prank(address) external;
+This is the crux of the issue I am reporting here. That's true in the case of a single position, the issue is that `funding_received` of that position being set to 0 does not mitigate the underpayment of funding which it made by going negative - they are opposite tokens in the pair.
+
+While that positions `funding_payment` is capped at the collateral of the specific position, the other side of the pool will continue to accrue funding receipts from this portion of the collateral until it's liquidated:
+
+```vyper
+ paid_long_term : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
+ received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
+```
+
+This is because this collateral is not excluded from `fs.long_collateral`, it must be explicitly subtracted upon liquidation.
+
+Now the issue causing loss of funds on this side is that positions will fail to close. On the other side, where `funding_received < funding_paid`, this is especially problematic in cases where the full funding payment was made, but the collateral fell short of the borrow fee.
+
+In this case, the balance `sum(funding_received) = sum(funding_paid)` was not broken by this position, as it made its full funding payment, but it will be excluded from receiving its portion of funding receipts. These tokens will not be directly claimable by any positions in the pool, causing loss of funds in that sense.
+
+Upholding this balance of funding payments to receipts is an important invariant which causes loss of funds and protocol disruptions as outlined above. This is even acknowledged by the team, since this current approach is meant to build up an "insurance" by penalizing negative positions to pay out future negative positions.
+
+What it fails to acknowledge is that the penalized position has already underpaid its funding fee (in token A), and now the balance is further distorted by eliminating its funding payment (in token B). To move closer to equilibrium between the two sides of funding, a direct approach such as socializing fee underpayments is recommended instead.
+
+There are a few different moving parts so let me know if any component of this line of reasoning is unclear and I could provide specific examples if needed.
+
+**WangSecurity**
- // Sets all subsequent calls' msg.sender to be the input address
- // until `stopPrank` is called
- function startPrank(address) external;
+> Doesn't the below rule from the severity categorization apply here? That's what I had in mind when creating the issue, since this comment was the only source of documentation on this specification. There is no indication that this is out of date based on any other specs or code elsewhere
- // Sets the *next* call's msg.sender to be the input address,
- // and the tx.origin to be the second input
- function prank(address, address) external;
+I didn't say the rule applies here, but if there's something said in the code comments and the code doesn't follow it, the issue still has to have at least Medium severity to be valid:
+> The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.
- // Sets all subsequent calls' msg.sender to be the input address until
- // `stopPrank` is called, and the tx.origin to be the second input
- function startPrank(address, address) external;
+So if the issue breaks an invariant from code comments, but it doesn't have Medium severity, then it's invalid.
- // Resets subsequent calls' msg.sender to be `address(this)`
- function stopPrank() external;
+> This is the crux of the issue I am reporting here. That's true in the case of a single position, the issue is that funding_received of that position being set to 0 does not mitigate the underpayment of funding which it made by going negative - they are opposite tokens in the pair.
+While that positions funding_payment is capped at the collateral of the specific position, the other side of the pool will continue to accrue funding receipts from this portion of the collateral until it's liquidated:
- // Reads the current `msg.sender` and `tx.origin` from state and reports if there is any active caller modification
- function readCallers() external returns (CallerMode callerMode, address msgSender, address txOrigin);
+That still doesn't prove how `funding_recevied` can be > `funding_paid`. The function [`calc_fees`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/positions.vy#L244) which is where this calculation of `funding_received` and `funding_paid` happens is called inside [`value`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/positions.vy#L162) function which is called only inside [`close`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/positions.vy#L389) and [`is_liquidatable`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/positions.vy#L356) , which is called only inside [`liquidate`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/core.vy#L325).
- // Sets an address' balance
- function deal(address who, uint256 newBalance) external;
-
- // Sets an address' code
- function etch(address who, bytes calldata code) external;
+Hence, this calculation of funding paid and received will be made only when closing or liquidating the position. So, the following is wrong:
+> the other side of the pool will continue to accrue funding receipts from this portion of the collateral until it's liquidated
- // Marks a test as skipped. Must be called at the top of the test.
- function skip(bool skip) external;
+It won't accrue because the position is either already liquidated or closed. Hence, the scenario of `funding_received > funding_paid` is still not proven.
- // Expects an error on next call
- function expectRevert() external;
- function expectRevert(bytes calldata) external;
- function expectRevert(bytes4) external;
+About the `funding_received < funding_paid`. As I understand, it's intended that the position doesn't receive any funding fees in this scenario which evident by [code comment](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/positions.vy#L259).
- // Record all storage reads and writes
- function record() external;
+So if the position didn't manage to pay the full `funding_paid` (underpaid the fees), they're intentionally excluded from receiving any funding fees and it's not a loss of funds and these will be received by other users.
- // Gets all accessed reads and write slot from a recording session,
- // for a given address
- function accesses(address)
- external
- returns (bytes32[] memory reads, bytes32[] memory writes);
-
- // Record all account accesses as part of CREATE, CALL or SELFDESTRUCT opcodes in order,
- // along with the context of the calls.
- function startStateDiffRecording() external;
+Hence, my decision remains: accept the escalation and invalidate the issue. If you still see that I'm wrong somewhere, you're welcome to correct me. But, to make it easier, provide links to the appropriate LOC you refer to.
- // Returns an ordered array of all account accesses from a `startStateDiffRecording` session.
- function stopAndReturnStateDiff() external returns (AccountAccess[] memory accesses);
- // Record all the transaction logs
- function recordLogs() external;
- // Gets all the recorded logs
- function getRecordedLogs() external returns (Log[] memory);
+**msheikhattari**
- // Prepare an expected log with the signature:
- // (bool checkTopic1, bool checkTopic2, bool checkTopic3, bool checkData).
- //
- // Call this function, then emit an event, then call a function.
- // Internally after the call, we check if logs were emitted in the expected order
- // with the expected topics and data (as specified by the booleans)
- //
- // The second form also checks supplied address against emitting contract.
- function expectEmit(bool, bool, bool, bool) external;
- function expectEmit(bool, bool, bool, bool, address) external;
+> Hence, this calculation of funding paid and received will be made only when closing or liquidating the position. So, the following is wrong:
- // Mocks a call to an address, returning specified data.
- //
- // Calldata can either be strict or a partial match, e.g. if you only
- // pass a Solidity selector to the expected calldata, then the entire Solidity
- // function will be mocked.
- function mockCall(address, bytes calldata, bytes calldata) external;
+That's not quite correct. Yes, `calc_fees` is only called upon closing/liquidating the position. But, this only only calculates the user's [pro-rate share of the accrued interest](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/fees.vy#L265):
- // Reverts a call to an address, returning the specified error
- //
- // Calldata can either be strict or a partial match, e.g. if you only
- // pass a Solidity selector to the expected calldata, then the entire Solidity
- // function will be mocked.
- function mockCallRevert(address where, bytes calldata data, bytes calldata retdata) external;
+```vyper
+def calc(id: uint256, long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
+ period: Period = self.query(id, opened_at)
+ P_b : uint256 = self.apply(collateral, period.borrowing_long) if long else (
+ self.apply(collateral, period.borrowing_short) )
+ P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
+ self.apply(collateral, period.funding_short) )
+ R_f : uint256 = self.multiply(collateral, period.received_long) if long else (
+ self.multiply(collateral, period.received_short) )
- // Clears all mocked and reverted mocked calls
- function clearMockedCalls() external;
- // Expect a call to an address with the specified calldata.
- // Calldata can either be strict or a partial match
- function expectCall(address callee, bytes calldata data) external;
- // Expect a call to an address with the specified
- // calldata and message value.
- // Calldata can either be strict or a partial match
- function expectCall(address callee, uint256, bytes calldata data) external;
+ return SumFees({funding_paid: P_f, funding_received: R_f, borrowing_paid: P_b})
+```
- // Gets the _creation_ bytecode from an artifact file. Takes in the relative path to the json file
- function getCode(string calldata) external returns (bytes memory);
- // Gets the _deployed_ bytecode from an artifact file. Takes in the relative path to the json file
- function getDeployedCode(string calldata) external returns (bytes memory);
+The terms `period.received_long`, `period.received_short` are relevant here and these are continuously, globally updated upon any interaction by any user with the system. As a result, those positions will count towards the total collateral until explicitly closed, inflating `funding_received` beyond its true value.
- // Label an address in test traces
- function label(address addr, string calldata label) external;
-
- // Retrieve the label of an address
- function getLabel(address addr) external returns (string memory);
+> and it's not a loss of funds and these will be received by other users.
- // When fuzzing, generate new inputs if conditional not met
- function assume(bool) external;
+The point of this issue is that user's will not receive those lost funds. Since the global `funding_received` included the collateral of positions which were later excluded from receiving their fee, the eventual pro-rata distribution of fees upon closing the position will not be adjusted for this. Thus some portion of fees will remain unclaimed.
- // Set block.coinbase (who)
- function coinbase(address) external;
+The current approach is problematic with loss of funds and disrupted liquidation functionality. There are more direct ways to achieve the correct balance of funding payments to receipts.
- // Using the address that calls the test contract or the address provided
- // as the sender, has the next call (at this call depth only) create a
- // transaction that can later be signed and sent onchain
- function broadcast() external;
- function broadcast(address) external;
+**WangSecurity**
- // Using the address that calls the test contract or the address provided
- // as the sender, has all subsequent calls (at this call depth only) create
- // transactions that can later be signed and sent onchain
- function startBroadcast() external;
- function startBroadcast(address) external;
- function startBroadcast(uint256 privateKey) external;
+To finally confirm, the problem here is that these funds are just locked in the contract, and no one can get them, correct?
- // Stops collecting onchain transactions
- function stopBroadcast() external;
+**rickkk137**
- // Reads the entire content of file to string, (path) => (data)
- function readFile(string calldata) external returns (string memory);
- // Get the path of the current project root
- function projectRoot() external returns (string memory);
- // Reads next line of file to string, (path) => (line)
- function readLine(string calldata) external returns (string memory);
- // Writes data to file, creating a file if it does not exist, and entirely replacing its contents if it does.
- // (path, data) => ()
- function writeFile(string calldata, string calldata) external;
- // Writes line to file, creating a file if it does not exist.
- // (path, data) => ()
- function writeLine(string calldata, string calldata) external;
- // Closes file for reading, resetting the offset and allowing to read it from beginning with readLine.
- // (path) => ()
- function closeFile(string calldata) external;
- // Removes file. This cheatcode will revert in the following situations, but is not limited to just these cases:
- // - Path points to a directory.
- // - The file doesn't exist.
- // - The user lacks permissions to remove the file.
- // (path) => ()
- function removeFile(string calldata) external;
- // Returns true if the given path points to an existing entity, else returns false
- // (path) => (bool)
- function exists(string calldata) external returns (bool);
- // Returns true if the path exists on disk and is pointing at a regular file, else returns false
- // (path) => (bool)
- function isFile(string calldata) external returns (bool);
- // Returns true if the path exists on disk and is pointing at a directory, else returns false
- // (path) => (bool)
- function isDir(string calldata) external returns (bool);
-
- // Return the value(s) that correspond to 'key'
- function parseJson(string memory json, string memory key) external returns (bytes memory);
- // Return the entire json file
- function parseJson(string memory json) external returns (bytes memory);
- // Check if a key exists in a json string
- function keyExists(string memory json, string memory key) external returns (bytes memory);
- // Get list of keys in a json string
- function parseJsonKeys(string memory json, string memory key) external returns (string[] memory);
+as I got the main point in this report is funding_received can exceed funding_paid but this isn't correct
- // Snapshot the current state of the evm.
- // Returns the id of the snapshot that was created.
- // To revert a snapshot use `revertTo`
- function snapshot() external returns (uint256);
- // Revert the state of the evm to a previous snapshot
- // Takes the snapshot id to revert to.
- // This deletes the snapshot and all snapshots taken after the given snapshot id.
- function revertTo(uint256) external returns (bool);
- // Creates a new fork with the given endpoint and block,
- // and returns the identifier of the fork
- function createFork(string calldata, uint256) external returns (uint256);
- // Creates a new fork with the given endpoint and the _latest_ block,
- // and returns the identifier of the fork
- function createFork(string calldata) external returns (uint256);
+>It's possible for funding_received by one side of the pool to exceed the funding_paid by the other side, in the case that a position went negative.
- // Creates _and_ also selects a new fork with the given endpoint and block,
- // and returns the identifier of the fork
- function createSelectFork(string calldata, uint256)
- external
- returns (uint256);
- // Creates _and_ also selects a new fork with the given endpoint and the
- // latest block and returns the identifier of the fork
- function createSelectFork(string calldata) external returns (uint256);
+
+
+provided PoC
+
- // Takes a fork identifier created by `createFork` and
- // sets the corresponding forked state as active.
- function selectFork(uint256) external;
+```vyper
- // Returns the currently active fork
- // Reverts if no fork is currently active
- function activeFork() external returns (uint256);
+ def test_funding_received_cannot_exceed_than_funding_paid(core, BTC,mint_token, STX, lp_provider, LP, open, mint, short, positions, owner, pools, fees, oracle, api, long, close):
+ pools.CORE() == core
+ fees.CORE() == core
+ positions.CORE() == core
+ oracle.API() == api
+ core.API() == api
+ mint_token(BTC, btc(1000), lp_provider)
+ mint_token(BTC, btc(1000), short)
+ mint_token(STX, d(100000), long)
- // Updates the currently active fork to given block number
- // This is similar to `roll` but for the currently active fork
- function rollFork(uint256) external;
- // Updates the given fork to given block number
- function rollFork(uint256 forkId, uint256 blockNumber) external;
- // Fetches the given transaction from the active fork and executes it on the current state
- function transact(bytes32) external;
- // Fetches the given transaction from the given fork and executes it on the current state
- function transact(uint256, bytes32) external;
+ mint_token(STX, d(100_000), lp_provider)
+ assert BTC.balanceOf(lp_provider) == btc(1000)
+ assert BTC.balanceOf(short) == btc(1000)
- // Marks that the account(s) should use persistent storage across
- // fork swaps in a multifork setup, meaning, changes made to the state
- // of this account will be kept when switching forks
- function makePersistent(address) external;
- function makePersistent(address, address) external;
- function makePersistent(address, address, address) external;
- function makePersistent(address[] calldata) external;
- // Revokes persistent status from the address, previously added via `makePersistent`
- function revokePersistent(address) external;
- function revokePersistent(address[] calldata) external;
- // Returns true if the account is marked as persistent
- function isPersistent(address) external returns (bool);
+ assert STX.balanceOf(lp_provider) == d(100_000)
- /// Returns the RPC url for the given alias
- function rpcUrl(string calldata) external returns (string memory);
- /// Returns all rpc urls and their aliases `[alias, url][]`
- function rpcUrls() external returns (string[2][] memory);
-}
+ core.fresh("BTC-STX", BTC, STX, LP, sender=owner)
+ BTC.approve(core.address, btc(1000), sender=lp_provider)
+ STX.approve(core.address, d(100_000), sender=lp_provider)
+ mint(BTC, STX, LP, btc(100), d(100_000), price=d(50_000), sender=lp_provider)
-```
+ BTC.approve(core.address, 10000000, sender=short)
+ STX.approve(core.address, d(100000), sender=long)
+ print("stx_balance:", STX.balanceOf(long) / 1e6)
-
+ open(BTC, STX, True, d(50000), 1, price=d(50_000), sender=long)
+ open(BTC, STX, False, 10000, 1, price=d(50_000), sender=short)
-
-ILeverage.sol
-```solidity
-// SPDX-License-Identifier: MIT
-pragma solidity >=0.8.13;
+ chain.mine(20000)
-interface ILeverage {
- function update() external;
- function calc() external returns(SumFees memory);
- function calc2() external returns(SumFees memory);
-
- struct SumFees{
- uint256 total;
- uint256 funding_paid;
- uint256 funding_received;
- uint256 borrowing_paid;
- }
+ position = positions.value(1, ctx(d(50_000)))#funding_fee payer
+ position2 = positions.value(2, ctx(d(50_000)))#funding_fee receiver
- struct FeeState{
- uint256 t1;
- uint256 funding_long;
- uint256 borrowing_long;
- uint256 funding_short;
- uint256 borrowing_short;
- uint256 long_collateral;
- uint256 short_collateral;
- uint256 borrowing_long_sum;
- uint256 borrowing_short_sum;
- uint256 funding_long_sum;
- uint256 funding_short_sum;
- uint256 received_long_sum;
- uint256 received_short_sum;
- }
-}
-```
+ print("position.remaining_collateral:", position.pnl.remaining / 1e6)
+ print("position.fees.funding_paid:", position.fees.funding_paid / 1e6)
+ print("position.funding_received:", position2.fees.funding_received / 1e6)
+
+ #@>>>>>position.remaining_collateral: 0.0
+ #@>>>>>position.fees.funding_paid: 50000.0
+ #@>>>>>position.funding_received: 50000.0
+```
-## Tool used
+**WangSecurity**
-Manual Review
+Yeah, there isn't a proof it can happen, the problem we are discussing now is that in cases where funding received is larger than funding paid (can happen when the position is negative), the funding received would be stuck in the contract with no one being able to get them. @rickkk137 would the funding fees be distributed to other users in this case?
-## Recommendation
-Consider applying all static and dynamic fees on `collateral * leverage`. The accounting for each fee should be consistently updated to use the notional value if this change is implemented.
+**rickkk137**
-# Issue M-12: Usage of `tx.origin` to determine the user is prone to attacks
+In my poc the position finally become negative but funding received is equal to funding paid
+But funding receiving wouldn't stuck in contracts and will be distributed to later positions
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/82
+**WangSecurity**
-## Found by
-Bauer, Greed, Japy69, KupiaSec, Waydou, bughuntoor, ctf\_sec, y4y
-## Summary
-Usage of `tx.origin` to determine the user is prone to attacks
+Thank you for this correction. With this, the escalation will be accepted and the issue will be invalidated. The decision will be applied in a couple of hours.
-## Vulnerability Detail
-Within `core.vy` to user on whose behalf it is called is fetched by using `tx.origin`.
-```vyper
- self._INTERNAL()
+**msheikhattari**
- user : address = tx.origin
-```
+Hi @rickkk137 can you specifically clarify the scenario which the PoC is describing? Because from my understanding, it is not showing the case which I described. The nuance is subtle, allow me to explain
-This is dangerous, as any time a user calls/ interacts with an unverified contract, or a contract which can change implementation, they're put under risk, as the contract can make a call to `api.vy` and act on user's behalf.
+Let's say there is one position on the long side, two on the short side, and that longs are paying shorts. Now if one of the short positions goes negative, the other will not receive the total funding fees over the period, it will only get its pro-rata share from the time that the (now liquidated) position was still active.
-Usage of `tx.origin` would also break compatibility with Account Abstract wallets.
+For comparison, it seems your PoC is showing a single long and short, and you are showing that when the long is liquidated the short should still receive their funding payment. This is a completely different scenario from what is described in this issue.
-## Impact
-Any time a user calls any contract on the BOB chain, they risk getting their funds lost.
-Incompatible with AA wallets.
+To answer your earlier question @WangSecurity
-## Code Snippet
-https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/core.vy#L166
+> To finally confirm, the problem here is that these funds are just locked in the contract, and no one can get them, correct?
-## Tool used
+Yes, thats correct. That portion of the funding payments is not accessible to any users.
-Manual Review
+**rickkk137**
-## Recommendation
-Instead of using `tx.origin` in `core.vy`, simply pass `msg.sender` as a parameter from `api.vy`
+when a position be penalized and funding_received for that position become zero and base or quote collateral's pool will be decrease
+```
+@external
+def close(id: uint256, d: Deltas) -> PoolState:
+...
+ base_collateral : self.MATH.eval(ps.base_collateral, d.base_collateral),
+ quote_collateral : self.MATH.eval(ps.quote_collateral, d.quote_collateral),
+```
+its mean other position get more funding_received compared to past because funding_recieved has reserve relation with base or qoute collateral
+```
+ paid_long_term : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
+ @>> received_short_term : uint256 = self.divide(paid_long_term, fs.short_collateral)
+```
-# Issue M-13: Funding Paid != Funding Received
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/83
-## Found by
-0xbranded
-## Summary
-Due to special requirements around receiving funding fees for a position, the funding fees received can be less than that paid. These funding fee payments are still payed, but a portion of them will not be withdrawn, and become stuck funds. This also violates the contract specification that `sum(funding_received) = sum(funding_paid)`.
+**msheikhattari**
-## Vulnerability Detail
-In [`calc_fees`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L257-L263) there are two special conditions that impact a position's receipt of funding payments:
+The pro rata share of funding received will be correctly adjusted moving forward from liquidation. But the point is that the period.funding_received terms already included the now liquidated collateral, so the other positions do not receive adjusted distributions to account for that.
+
+**rickkk137**
-```vyper
- # When there are negative positions (liquidation bot failure):
- avail : uint256 = pool.base_collateral if pos.long else (
- pool.quote_collateral )
- # 1) we penalize negative positions by setting their funding_received to zero
- funding_received: uint256 = 0 if remaining == 0 else (
- # 2) funding_received may add up to more than available collateral, and
- # we will pay funding fees out on a first come first serve basis
- min(fees.funding_received, avail) )
```
+def query(id: uint256, opened_at: uint256) -> Period:
+ """
+ Return the total fees due from block `opened_at` to the current block.
+ """
+ fees_i : FeeState = Fees(self).fees_at_block(opened_at, id)
+ fees_j : FeeState = Fees(self).current_fees(id)
+ return Period({
+ borrowing_long : self.slice(fees_i.borrowing_long_sum, fees_j.borrowing_long_sum),
+ borrowing_short : self.slice(fees_i.borrowing_short_sum, fees_j.borrowing_short_sum),
+ funding_long : self.slice(fees_i.funding_long_sum, fees_j.funding_long_sum),
+ funding_short : self.slice(fees_i.funding_short_sum, fees_j.funding_short_sum),
+ @>>> received_long : self.slice(fees_i.received_long_sum, fees_j.received_long_sum),
+ @>>> received_short : self.slice(fees_i.received_short_sum, fees_j.received_short_sum),
+ })
+both will be updated when liquidable position will be closed
+
-If the position has run out of collateral by the time it is being closed, he will receive none of his share of funding payments. Additionally, if the available collateral is not high enough to service the funding fee receipt, he will receive only the greatest amount that is available.
+**msheikhattari**
-These funding fee payments are still always made ([deducted](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L250) from remaining collateral), whether they are received or not:
+That's not quite right. From [`current_fees`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/fees.vy#L137):
```vyper
- c1 : Val = self.deduct(c0, fees.funding_paid)
+ paid_short_term : uint256 = self.apply(fs.short_collateral, fs.funding_short * new_terms)
+ received_long_term : uint256 = self.divide(paid_short_term, fs.long_collateral)
+ received_long_sum : uint256 = self.extend(fs.received_long_sum, received_long_term, 1)
```
-When a position is closed under most circumstances, the pool will have enough collateral to service the corresponding fee payment:
+So as mentioned in my point above, the collateral at the time of each global fee update is used. When a user later claims his pro-rate share, he will receive his fraction of the total collateral *at the time of the fee update*. Fee updates are performed continuously after each operation, and the total collateral may no longer be representative due to liquidated positions being removed from this sum. However, they were still included in the `received_{short/long}_term`, which is added onto the globally stored `received_{short/long}_sum`
-```vyper
-# longs
-base_collateral : [self.MATH.MINUS(fees.funding_received)],
-quote_collateral: [self.MATH.PLUS(fees.funding_paid),
- self.MATH.MINUS(pos.collateral)],
-...
-# shorts
-base_collateral : [self.MATH.PLUS(fees.funding_paid), # <-
- self.MATH.MINUS(pos.collateral)],
-quote_collateral: [self.MATH.MINUS(fees.funding_received)],
-```
+Thus some share of these global fees are not accessible.
-When positions are closed, the original collateral (which was placed into the pool upon opening) is removed. However, the amount of funding payments a position made is added to the pool for later receipt. Thus, when positions are still open there is enough position collateral to fulfill the funding fee payment and when they close the funding payment made by that position still remains in the pool.
+**WangSecurity**
-Only when the amount of funding a position paid exceeded its original collateral, will there will not be enough collateral to service the receipt of funding fees, as alluded to in the comments. However, it's possible for users to pay the full funding fee, but if the borrow fee exceeds the collateral balance remaining thereafter, they will not receive any funding fees. As a result, it's possible for funding fees to be paid which are never received.
+As I understand it:
+The fee state is checked twice in the liquidate/close. Let's take close for example:
+1. The state is checked during the [close](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/core.vy#L290) in the positions. It doesn't update the state and calculates the funding_received and funding_paid (well, in fact, it calls `close`, then it calls `value`, which calls `calc_fees`) which gives us 0 funding_received and thus 0 added to the quote_collateral.
+2. Then `core::close` updates the Pool and then updates the [fees](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/core.vy#L294).
+3. When we update the fees, we use [`FeeState.base_collateral`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/fees.vy#L186) (assuming the scenario with two shorts) when calculating the fees received by shorts for this term. But, the `FeeState.base collateral` is changed only [after calculating `received_short_term/sum`](https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/fees.vy#L226)
-Further, even in the case of funding fee underpayment, setting the funding fee received to 0 does not remedy this issue. The funding fees which he underpaid were in a differing token from those which he would receive, so this only furthers the imbalance of fees received to paid.
+So even though the closed/liquidated position didn't receive any fees and they should go to another short, the `received_short_term` accounted as there were two shorts opened, and each received their funding fees.
-## Impact
-`core.vy` includes a specification for one of the invariants of the protocol:
-```vyper
-# * funding payments match
-# sum(funding_received) = sum(funding_paid)
-```
+Hence, when the other short position gets the fees, they will add only a portion from that period, not the full fee.
-This invariant is clearly broken as some portion of paid funding fees will not be received under all circumstances, so code is not to spec. This will also lead to some stuck funds, as a portion of the paid funding fees will never be deducted from the collateral. This can in turn lead to dilution of fees for future funding fee recipients, as the payments will be distributed evenly to the entire collateral including these stuck funds which will never be removed.
-## Code Snippet
+Thus, I agree it should remain valid. However, medium severity should be kept because, in reality, if this situation occurs, the collateral of that closing/liquidatable position would be quite low (lower than `funding_paid`), there would be many positions, so the individual loss would be smaller (in terms of the amount, not %), the period with incorrectly applied fees would be small (given the fact that fees are updated at each operation). Hence, the loss is very limited. Planning to reject the escalation and leave the issue as it is, it will be applied at 10 am UTC.
-## Tool used
+**WangSecurity**
-Manual Review
+Result:
+Medium
+Unique
-## Recommendation
-Consider an alternative method of accounting for funding fees, as there are many cases under the current accounting where fees received/paid can fall out of sync.
+**sherlock-admin4**
-For example, include a new state variable that explicitly tracks unpaid funding fee payments and perform some pro rata or market adjustment to future funding fee recipients, specifically for *that token*.
+Escalations have been resolved successfully!
+
+Escalation status:
+- [spacegliderrrr](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/83/#issuecomment-2344430632): rejected
+
+**WangSecurity**
-# Issue M-14: First depositor could DoS the pool
+#93 and #18 are duplicates of this issue; they both identify that if the negative position is closed, the funding_received for it reset to 0, but these funding fees are not received by any other user and remain stuck in the contract forever.
+
+# Issue M-8: First depositor could DoS the pool
Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/85
@@ -3724,59 +2966,129 @@ Manual Review
## Recommendation
Add a minimum liquidity requirement.
-# Issue M-15: Liquidity providers can remove liquidity to force positions into high fees
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/88
-## Found by
-4gontuk, bughuntoor
-## Summary
-Liquidity providers can remove liquidity to force positions into high fees
+## Discussion
-## Vulnerability Detail
-Within the protocol, position fees are based on utilization of the total LP amount.
+**msheikhattari**
+
+Escalate
+
+> DoS has two separate scores on which it can become an issue:
+> 1. The issue causes locking of funds for users for more than a week (overrided to 4 hr)
+> 2. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.
+Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.
+
+Low at best. Per the severity guidelines, this is not DoS since no user funds are locked and no sensitive functionality is impacted (ongoing positions/LPs are not impacted). Additionally, this both assumes that no other LPs make any deposits within the same block as the attacker (as the price would be equivalent), and that the price is monotonically decreasing after the attack was initiated. Not only is it low impact, but also low likelihood.
+
+**sherlock-admin3**
+
+> Escalate
+>
+> > DoS has two separate scores on which it can become an issue:
+> > 1. The issue causes locking of funds for users for more than a week (overrided to 4 hr)
+> > 2. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.
+> Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.
+>
+> Low at best. Per the severity guidelines, this is not DoS since no user funds are locked and no sensitive functionality is impacted (ongoing positions/LPs are not impacted). Additionally, this both assumes that no other LPs make any deposits within the same block as the attacker (as the price would be equivalent), and that the price is monotonically decreasing after the attack was initiated. Not only is it low impact, but also low likelihood.
+
+You've created a valid escalation!
+
+To remove the escalation from consideration: Delete your comment.
+
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
+
+**WangSecurity**
+
+While indeed this doesn't qualify the DOS requirements, this issue can still result in the functionality of the protocol being blocked and the users wouldn't be able to use this pool. I agree it's low likelihood, but it's still possible.
+
+@spacegliderrrr since this issue relies on the precision loss, it requires a POC, can you make one? And also how low should be the price so it leads to rounding down?
+
+**spacegliderrrr**
+
+Price doesn't really matter - it's just needed to deposit little enough tokens that they're worth 1 wei of quote token. So if for example the pair is WETH/ USDC, a user would need to deposit ~4e8 wei WETH (considering price of $2,500).
+
+As for the PoC, because the code is written in Vyper and Foundry does not support it, I cannot provide a PoC.
+
+**WangSecurity**
+
+In that case, could you make a very detailed attack path, I see you provided an example of the price that would cause an issue, but still would like a very detailed attack path.
+
+**WangSecurity**
+
+One important thing to consider is the following question:
+> We will report issues where the core protocol functionality is inaccessible for at least 7 days. Would you like to override this value?
+Yes, 4 hours
+
+So, I see that DOS rules say that the funds should be locked for a week. But, the question is about core protocol functionality being inaccessible. The protocol specified they want to have issues about core protocol functionality being inaccessible for 4 hours.
+
+This finding, indeed doesn't lead to locking funds or blocking time-sensitive functions, but it can lead to the core protocol functionality being inaccessible for 4 hours. I see that it's low likelihood, but the likelihood is not considered when defining the severity. Hence, even though for this finding there have to be no other depositors in the next block or the price being lower for the next 4 hours, this can happen. Thus, medium severity for this issue is appropriate. Planning to reject the escalation and leave the issue as it is.
+
+**msheikhattari**
+
+This issue doesn't impact ongoing operations, so its similar to frontrunning of initializers. No irreversible damage or loss of funds occur.
+
+Core protocol functionality being inaccessible should have some ramifications like lock of funds or broken time-sensitive functionality (like withdrawals). No funds are in the pool when this issue is taking place.
+
+**msheikhattari**
+
+In any case this issue does need a PoC - per the severity criteria all issues related to precision loss require one.
+
+**WangSecurity**
+
+@spacegliderrrr could you make a detailed numerical POC, showcasing the precision loss and DOS.
+
+> Core protocol functionality being inaccessible should have some ramifications like lock of funds or broken time-sensitive functionality (like withdrawals). No funds are in the pool when this issue is taking place
+
+As I've said previously, this still impacts the core protocol functionality, as the users cannot deposit into the pool, and this very well could last for more than 4 hours. Hence, this is sufficient for medium severity as the core protocol functionality is inaccessible for more than 4 hours.
+
+**spacegliderrrr**
+
+1. Market pair used is WETH/USDT. Current WETH price $2,500.
+2. User is the first depositor in the pool, depositing 4e8 WETH. Based on the lines of code below, the user is minted 1 lp token.:
+`amt0 = 4e8 * 2500e18 / 1e18 = 10000e8 = 1e12`
+`lowered = 1e12 / 1e12 = 1`
```vyper
-def dynamic_fees(pool: PoolState) -> DynFees:
- long_utilization : uint256 = self.utilization(pool.base_reserves, pool.base_interest)
- short_utilization: uint256 = self.utilization(pool.quote_reserves, pool.quote_interest)
- borrowing_long : uint256 = self.check_fee(
- self.scale(self.PARAMS.MAX_FEE, long_utilization))
- borrowing_short : uint256 = self.check_fee(
- self.scale(self.PARAMS.MAX_FEE, short_utilization))
- funding_long : uint256 = self.funding_fee(
- borrowing_long, long_utilization, short_utilization)
- funding_short : uint256 = self.funding_fee(
- borrowing_short, short_utilization, long_utilization)
- return DynFees({
- borrowing_long : borrowing_long,
- borrowing_short: borrowing_short,
- funding_long : funding_long,
- funding_short : funding_short,
- })
+@external
+@pure
+def base_to_quote(tokens: uint256, ctx: Ctx) -> uint256:
+ lifted : Tokens = self.lift(Tokens({base: tokens, quote: ctx.price}), ctx)
+ amt0 : uint256 = self.to_amount(lifted.quote, lifted.base, self.one(ctx))
+ lowered: Tokens = self.lower(Tokens({base: 0, quote: amt0}), ctx)
+ return lowered.quote
+```
+3. Next block comes and WETH price drops to $2,499.
+4. A user now attempts to deposit. Since LP tokens minted are respective to the current pool value, contract calculates pool value, using the same formula above
+`amt0 = 4e8 * 2499e18 / 1e18 = 0.9996e12`
+`lowered = 0.9996e12 / 1e12 = 0`
+5. User's LP tokens are calculated using the following formula. As the pool's value is 0, a division by 0 is attempted, which forces the tx to revert.
+```vyper
+def f(mv: uint256, pv: uint256, ts: uint256) -> uint256:
+ if ts == 0: return mv
+ else : return (mv * ts) / pv
```
-The problem is that this number is dynamic and LP providers can abuse it at any point
+6. Pool is DoS'd until ETH price goes above $2,500 again.
-Consider the following scenario:
-1. There's a lot of liquidity in the pool
-2. User opens a position for a fraction of it, expecting low fees due the low utilization
-3. LP providers withdraw most of liquidity, forcing high utilization ratio and ultimately high fees.
-## Impact
-Loss of funds
+**WangSecurity**
+As far as I can tell, the POC is correct and this issue will indeed happen. As was said previously, planning to reject the escalation and leave the issue as it is.
-## Code Snippet
-https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L33
+**WangSecurity**
-## Tool used
+Result:
+Medium
+Unique
-Manual Review
+**sherlock-admin4**
-## Recommendation
-Consider using a different way to calculate fees which is not manipulatable by the LP providers.
+Escalations have been resolved successfully!
-# Issue M-16: Whale LP providers can open positions on both sides to force users into high fees.
+Escalation status:
+- [msheikhattari](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/85/#issuecomment-2345044761): rejected
+
+# Issue M-9: Whale LP providers can open positions on both sides to force users into high fees.
Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/89
@@ -3809,146 +3121,87 @@ Manual Review
## Recommendation
Consider a different way to calculate fees
-# Issue M-17: Loss of Funds From Profitable Positions Running Out of Collateral
-
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/90
-
-## Found by
-0xbranded, Yashar
-## Summary
-When profitable positions run out of collateral, they receive no payout, even if they had a positive PnL. This is not only disadvantageous to users, but it critically removes all liquidation incentive. These zero'd out positions will continue to underpay fees until someone liquidates the position for no fee, losing money to gas in the process.
-
-## Vulnerability Detail
-When calculating the pnl of either a long or short position that is to be closed, if the collateral drops to zero due to fee obligations then they [do not receive a payout](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L295):
-
-```vyper
-# remaining first calculating in `calc_fees`
- c0 : uint256 = pos.collateral
- c1 : Val = self.deduct(c0, fees.funding_paid)
- c2 : Val = self.deduct(c1.remaining, fees.borrowing_paid)
- # Funding fees prioritized over borrowing fees.
- funding_paid : uint256 = c1.deducted
- borrowing_paid : uint256 = c2.deducted
- remaining : uint256 = c2.remaining
-
-...
-# later passed to `calc_pnl`
- final : uint256 = 0 if remaining == 0 else (
-...
-# longs
- payout : uint256 = self.MATH.quote_to_base(final, ctx)
-...
-# shorts
- payout : uint256 = final
-```
-
-However, it's possible for a position to run out of collateral yet still have a positive PnL. In these cases, no payout is received. This [payout is what's sent](https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L195) to the user **or liquidator** when a position is closed:
-
-```vyper
-# longs
- base_transfer : [self.MATH.PLUS(pnl.payout),
-# shorts
- quote_transfer : [self.MATH.PLUS(pnl.payout),
-```
-
-In these cases neither the user closing his position, nor the liquidator receives any payment.
-## Impact
-While it may be intended design to penalize users for letting a position run out of collateral, this is a dangerous design choice. It's possible to end up in a situation where a position has run negative due to the funding fees and now has no incentive for liquidation. This will be the case even if it could have been profitable to liquidate this position due to the PnL of the position.
-
-This scenario is dependent on liquidation bots malfunctioning since the liquidatability of a position does not factor in profit (only losses). However, it is acknowledged as a possibility that this scenario may occur throughout the codebase as safeguards are introduced to protect against this scenario elsewhere. In this particular scenario, no particular safeguard exists.
-These positions will continue to decay causing further damage to the system until someone is willing to liquidate the position for no payment. It is unlikely that a liquidation bot would be willing to lose money to do so and would likely require admin intervention. By the time admin intervenes, it's most likely that further losses would have already resulted from the decay of the position.
+## Discussion
-## Code Snippet
+**msheikhattari**
-## Tool used
+Escalate
+Invalid. Quoting a valid point from your own [comment](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/28#issuecomment-2344387728):
+> Issue should be low/info. Ultimately, all LPs would want is fees and this would give them the highest fees possible. Furthermore, the attack is extremely costly, as it would require user to lock up hundreds of thousands/ millions, losing a significant % of them. Any user would have an incentive to add liquidity at extremely high APY, which would allow for both new positions opens and LP withdraws.
-Manual Review
+This attack inflates borrow fees, but the high APY will attract other LP depositors which would drive the utilization back down to normal levels, reducing the fee. Unlike the issue that you were escalating, this one has no such time sensitivity - the market would naturally tend towards rebalance within the next several days / weeks. It's not reasonable to assume that the existing positions would remain open despite high fees and other LPs would not enter the market over the coming days/weeks.
-## Recommendation
-During liquidations, provide the liquidator with the remaining PnL even if the position has run negative due to fees. This will maintain the current design of penalizing negative positions while mitigating the possibility of positions with no liquidation incentive.
+Not only that, the other assumptions of this issue are incorrect:
+> If at any point they can no longer afford maintaining majority, they can simply close their positions without taking a loss, so this is basically risk-free.
-Alternatively, include a user's PnL in his payout even if the collateral runs out. This may not be feasible due to particular design choices to ensure the user doesn't let his position run negative.
+Wrong. Each opened long AND short position must pay a fixed fee, so the whale is taking a risk. He is betting that the current positions will not close, and his stake will not get diluted, just long enough to eke out a net profit. And this is assuming he had a majority stake to begin with, which for the more liquid pools where the attack is most profitable due to a large amount of open interest, is a highly questionable assumption.
-# Issue M-18: `base_to_quote` wrongly assume quote always has less decimals
+The game theory makes it unlikely that the whale would be able to extract enough extra fees to even make a profit net of the operating fees of such an attack.
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/91
+**sherlock-admin3**
-## Found by
-bareli, bughuntoor
-## Summary
-`base_to_quote` wrongly assume quote always has less decimals
+> Escalate
+> Invalid. Quoting a valid point from your own [comment](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/28#issuecomment-2344387728):
+> > Issue should be low/info. Ultimately, all LPs would want is fees and this would give them the highest fees possible. Furthermore, the attack is extremely costly, as it would require user to lock up hundreds of thousands/ millions, losing a significant % of them. Any user would have an incentive to add liquidity at extremely high APY, which would allow for both new positions opens and LP withdraws.
+>
+> This attack inflates borrow fees, but the high APY will attract other LP depositors which would drive the utilization back down to normal levels, reducing the fee. Unlike the issue that you were escalating, this one has no such time sensitivity - the market would naturally tend towards rebalance within the next several days / weeks. It's not reasonable to assume that the existing positions would remain open despite high fees and other LPs would not enter the market over the coming days/weeks.
+>
+> Not only that, the other assumptions of this issue are incorrect:
+> > If at any point they can no longer afford maintaining majority, they can simply close their positions without taking a loss, so this is basically risk-free.
+>
+> Wrong. Each opened long AND short position must pay a fixed fee, so the whale is taking a risk. He is betting that the current positions will not close, and his stake will not get diluted, just long enough to eke out a net profit. And this is assuming he had a majority stake to begin with, which for the more liquid pools where the attack is most profitable due to a large amount of open interest, is a highly questionable assumption.
+>
+> The game theory makes it unlikely that the whale would be able to extract enough extra fees to even make a profit net of the operating fees of such an attack.
-## Vulnerability Detail
-Let's look at the code of `base_to_quote`
+You've created a valid escalation!
-```vyper
-def base_to_quote(tokens: uint256, ctx: Ctx) -> uint256: # price is in quote decimals
- lifted : Tokens = self.lift(Tokens({base: tokens, quote: ctx.price}), ctx) # get scaled to whichever has more decimals
- amt0 : uint256 = self.to_amount(lifted.quote, lifted.base, self.one(ctx)) # amount gets calculated in whichever has more decimals
- lowered: Tokens = self.lower(Tokens({base: 0, quote: amt0}), ctx) # amount gets refactored to whichever has less decimals -> will be wrong if that's base
- return lowered.quote
-```
+To remove the escalation from consideration: Delete your comment.
-In the input params, `tokens` is in base decimals and the price is in `quote` decimals.
-As we can see, the first line scales to whichever has more decimals.
-Then the amount is calculated, once again scaled in whichever has more decimals.
-Then it is lowered to whichever has lower decimals.
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
-It basically makes an assumption that the quote always has less decimals, which is not the case. If the pair is `WBTC/DAI` for example, it would be completely broken and allow for draining all assets.
+**WangSecurity**
-## Impact
-Loss of funds.
+@spacegliderrrr do you have any counterarguments?
-## Code Snippet
-https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/math.vy#L73
+**spacegliderrrr**
-## Tool used
+@WangSecurity Issue above showcases a real issue which could occur if a whale decides to _attack_ a pool.
-Manual Review
+> Each opened long AND short position must pay a fixed fee, so the whale is taking a risk. He is betting that the current positions will not close, and his stake will not get diluted, just long enough to eke out a net profit.
-## Recommendation
-Do proper decimals scaling
+True, there's some risk, though most of it can be mitigated. For example if the attack is performed when most opened positions are at negative PnL (which would mean closing them is profitable to the LP providers), most of the risk is mitigated as users have 2 choices - close early at a loss or keep the position open at high fees (either way, profitable for the LP provider).
-# Issue M-19: User can sandwich their own position close to get back all of their position fees
+> the market would naturally tend towards rebalance within the next several days / weeks.
-Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/94
+True, though as mentioned, it would take days/ weeks in which the whale could profit.
-## Found by
-bughuntoor
-## Summary
-User can sandwich their own position close to get back all of their position fees
+The issue does involve some game theory, but nonetheless shows an actual risk to honest users.
-## Vulnerability Detail
-Within the protocol, borrowing fees are only distributed to LP providers when the position is closed. Up until then, they remain within the position.
-The problem is that in this way, fees are distributed evenly to LP providers, without taking into account the longevity of their LP provision.
+**WangSecurity**
-This allows a user to avoid paying fees in the following way:
-1. Flashloan a large sum and add it as liquidity
-2. Close their position and let the fees be distributed (with most going back to them as they've got majority in the pool)
-3. WIthdraw their LP tokens
-4. Pay back the flashloan
+I also agree there are lots of risks, with this scenario. But, it's still possible to pose losses on other users in a way of arbitrary increasing fees. The market would rebalance, but it can even take less than a day to cause losses to users. Hence, I agree that this issue should remain medium severity, because even though the issue has high constraints, still can cause losses. Planning to reject the escalation.
-## Impact
-Users can avoid paying borrowing fees.
+**WangSecurity**
-## Code Snippet
-https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L156
+Result:
+Medium
+Unique
-## Tool used
+**sherlock-admin4**
-Manual Review
+Escalations have been resolved successfully!
-## Recommendation
-Implement a system where fees are gradually distributed to LP providers.
+Escalation status:
+- [msheikhattari](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/89/#issuecomment-2345018904): rejected
-# Issue M-20: User could have impossible to close position if funding fees grow too big.
+# Issue M-10: User could have impossible to close position if funding fees grow too big.
Source: https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/96
## Found by
-0xbranded, bughuntoor
+bughuntoor
## Summary
User could have impossible to close position if funding fees grow too big.
@@ -3987,3 +3240,327 @@ Manual Review
## Recommendation
Fix is non-trivial.
+
+
+## Discussion
+
+**KupiaSecAdmin**
+
+Escalate
+
+The underflow does not happen by nature of `deduct` function. Thus this is invalid.
+
+**sherlock-admin3**
+
+> Escalate
+>
+> The underflow does not happen by nature of `deduct` function. Thus this is invalid.
+
+You've created a valid escalation!
+
+To remove the escalation from consideration: Delete your comment.
+
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
+
+**spacegliderrrr**
+
+Escalate
+
+Severity should be High. Issue above describes how a user could open risk-free max leverage positions, basically making a guaranteed profit from the LPs.
+
+Regarding @KupiaSecAdmin escalation above - please do double check the issue above. The underflow does not happen in `deduct` but rather in the `MATH.eval` operations. The problem lies within the fact that if order of withdraws is reversed, funding receiver can receive more fees than the total collateral (as long as it is available by other users who have said collateral not yet eaten up by funding fees). Then, some of the funding paying positions will be impossible to be closed.
+
+**sherlock-admin3**
+
+> Escalate
+>
+> Severity should be High. Issue above describes how a user could open risk-free max leverage positions, basically making a guaranteed profit from the LPs.
+>
+> Regarding @KupiaSecAdmin escalation above - please do double check the issue above. The underflow does not happen in `deduct` but rather in the `MATH.eval` operations. The problem lies within the fact that if order of withdraws is reversed, funding receiver can receive more fees than the total collateral (as long as it is available by other users who have said collateral not yet eaten up by funding fees). Then, some of the funding paying positions will be impossible to be closed.
+
+You've created a valid escalation!
+
+To remove the escalation from consideration: Delete your comment.
+
+You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
+
+**ami0x226**
+
+This is invalid.
+
+> 4. The original long is closed. This does not have an impact on the total quote collateral, as it is increased by the funding_paid which in our case will be counted as exactly as much as the collateral (as in these calculations it cannot surpass it). And it then subtracts that same quote collateral.
+
+When original long is closed, total quote collateral is changed.
+```rust
+File: gl-sherlock\contracts\positions.vy
+209: quote_reserves : [self.MATH.PLUS(pos.collateral), #does not need min()
+210: self.MATH.MINUS(fees.funding_paid)],
+211: quote_collateral: [self.MATH.PLUS(fees.funding_paid),
+212: self.MATH.MINUS(pos.collateral)],
+```
+Heres, `pos.collateral = X, fees.funding_paid = X + Y`.
+Then,
+`quote_collateral <- quote_collateral + X + Y - X = quote_collateral + Y = 2X + Y`, and
+`quote_reserves <- quote_reserves + X - X - Y = quote_reserves - Y`.
+
+When original short is closed in step5, new total quote collateral is `2X + Y - (X + Y) = X` and there is no underflow in step6.
+As a result, the scenario of the report is wrong.
+The loss causes in `quote_reserves`, but, in practice, Y is enough small by the frequent liquidation and it should be assumed that the liquidation is done correctly.
+Especially, because the report does not mention about this vulnerability, I think this is invalid
+
+**ami0x226**
+
+Also, Funding paid cannot exceed collateral of a position from the `apply` function.
+```vyper
+File: gl-sherlock\contracts\math.vy
+167: def apply(x: uint256, numerator: uint256) -> Fee:
+172: fee : uint256 = (x * numerator) / DENOM
+173: remaining: uint256 = x - fee if fee <= x else 0
+174: fee_ : uint256 = fee if fee <= x else x
+175: return Fee({x: x, fee: fee_, remaining: remaining})
+```
+
+```vyper
+File: gl-sherlock\contracts\fees.vy
+265: def calc(id: uint256, long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
+269: P_f : uint256 = self.apply(collateral, period.funding_long) if long else (
+270: self.apply(collateral, period.funding_short) )
+274: return SumFees({funding_paid: P_f, funding_received: R_f, borrowing_paid: P_b})
+```
+
+**spacegliderrrr**
+
+> Heres, pos.collateral = X, fees.funding_paid = X + Y.
+
+Here's where you're wrong. When the user closes their position, `funding_paid` cannot exceed `pos.collateral`. So `fees.funding_paid == pos.collateral` when closing the original long. Please re-read the issue and code again.
+
+**ami0x226**
+
+> > Heres, pos.collateral = X, fees.funding_paid = X + Y.
+>
+> Here's where you're wrong. When the user closes their position, `funding_paid` cannot exceed `pos.collateral`. So `fees.funding_paid == pos.collateral` when closing the original long. Please re-read the issue and code again.
+
+That's true. I mentioned about it in the [above comment](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/96#issuecomment-2345445382)
+> Also, Funding paid cannot exceed collateral of a position from the apply function.
+
+I just use `fees.funding_paid = X + Y` to follow the step2 of `bughuntoor`'s scenario:
+> 2. Eventually the funding fee grows larger than the whole long position (X + Y). it is due liquidation, but due to bot failure is not yet liquidated (which based on comments is expected and possible behaviour)
+
+
+**rickkk137**
+
+invalid
+funding_paid cannot exceed than collateral also funding_received cannot be greater funding_paid
+
+**WangSecurity**
+
+@spacegliderrrr can you make a coded POC showcasing the attack path from the report?
+
+**WangSecurity**
+
+We've got the POC from the sponsor:
+
+
+POC
+
+```python
+from ape import chain
+import pytest
+from conftest import d, ctx
+
+# https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/96
+
+# 1) There's an open long (for total collateral of X) and an open short position. Long position pays funding fee to the short position.
+# 2) Eventually the funding fee grows larger than the whole long position (X + Y). it is due liquidation, but due to bot failure is not yet liquidated (which based on comments is expected and possible behaviour)
+# 3) A new user opens a new long position, once with X collateral. (total quote collateral is currently 2X)
+# 4) The original long is closed. This does not have an impact on the total quote collateral, as it is increased by the funding_paid which in our case will be counted as exactly as much as the collateral (as in these calculations it cannot surpass it). And it then subtracts that same quote collateral.
+# 5) The original short is closed. funding_received is calculated as X + Y and therefore that's the amount the total quote collateral is reduced by. The new total quote collateral is 2X - (X + Y) = X - Y.
+# 6) Later when the user attempts to close their position it will fail as it will attempt subtracting (X - Y) - X which will underflow.
+
+# OR
+# 1. Consider the original long and short positions, long pays funding fee to short.
+# 2. Time goes by, liquidator bots fails and funding fee makes 100% of the long collateral (consider collateral is X)
+# 3. Another long position is opened again with collateral X.
+# 4. Time goes by, equivalent to funding fee of 10%. Total collateral at this moment is still 2X, so the new total funding paid is 1.2X
+# 5. Short position closes and receives funding paid of 1.2X. Quote collateral is now reduced from 2X to 0.8X.
+# 6. (Optional) Original long closes position. For them funding paid is capped at their collateral, so funding paid == collateral, so closing does not make a difference on the quote collateral.
+# 7. The next long position holder tries to close. They're unable because their collateral is 1x, funding paid is 0.1x. Collateral calculation is 0.8X + 0.1X - 1X and underflow reverts
+
+PARAMS = {
+ 'MIN_FEE' : 1_00000000,
+ 'MAX_FEE' : 1_00000000, # 10%/block
+ 'PROTOCOL_FEE' : 1000,
+ 'LIQUIDATION_FEE' : 2,
+ 'MIN_LONG_COLLATERAL' : 1,
+ 'MAX_LONG_COLLATERAL' : 1_000_000_000,
+ 'MIN_SHORT_COLLATERAL' : 1,
+ 'MAX_SHORT_COLLATERAL' : 1_000_000_000,
+ 'MIN_LONG_LEVERAGE' : 1,
+ 'MAX_LONG_LEVERAGE' : 10,
+ 'MIN_SHORT_LEVERAGE' : 1,
+ 'MAX_SHORT_LEVERAGE' : 10,
+ 'LIQUIDATION_THRESHOLD' : 1,
+}
+
+PRICE = 400_000
+
+def test_issue(core, api, pools, positions, fees, math, oracle, params,
+ VEL, STX, LP,
+ mint, burn, open, close,
+ long, short, lp_provider, long2, owner,
+ mint_token):
+
+ # setup
+ core.fresh("VEL-STX", VEL, STX, LP, sender=owner)
+ mint_token(VEL, d(100_000), lp_provider)
+ mint_token(STX, d(100_000), lp_provider)
+ mint_token(VEL, d(10_000) , long)
+ mint_token(STX, d(10_000) , long)
+ mint_token(VEL, d(10_000) , long2)
+ mint_token(STX, d(10_000) , long2)
+ mint_token(VEL, d(10_000) , short)
+ mint_token(STX, d(10_000) , short)
+ VEL.approve(core.address, d(100_000), sender=lp_provider)
+ STX.approve(core.address, d(100_000), sender=lp_provider)
+ VEL.approve(core.address, d(10_000) , sender=long)
+ STX.approve(core.address, d(10_000) , sender=long)
+ VEL.approve(core.address, d(10_000) , sender=long2)
+ STX.approve(core.address, d(10_000) , sender=long2)
+ VEL.approve(core.address, d(10_000) , sender=short)
+ STX.approve(core.address, d(10_000) , sender=short)
+ mint(VEL, STX, LP, d(10_000), d(4_000), price=PRICE, sender=lp_provider)
+ params.set_params(PARAMS, sender=owner) # set 10 % fee / block
+
+ START_BLOCK = chain.blocks[-1].number
+ print(f"Start block: {START_BLOCK}")
+
+ # 1) There's an open long (for total collateral of X) and an open short position. Long position pays funding fee to the short position.
+ # open pays funding when long utilization > short utilization (interest/reserves)
+ X = d(100)
+ p1 = open(VEL, STX, True , X , 10, price=PRICE, sender=long)
+ p2 = open(VEL, STX, False , d(5), 2, price=PRICE, sender=short)
+ assert not p1.failed, "open long"
+ assert not p2.failed, "open short"
+
+ fees = params.dynamic_fees(pools.lookup(1))
+ print(f"Pool fees: {fees}")
+
+ # 2. Time goes by, liquidator bots fails and funding fee makes 100% of
+ # the long collateral (consider collateral is X)
+ chain.mine(10)
+
+ # fees/value after
+ value = positions.value(1, ctx(PRICE))
+ print(value['fees'])
+ print(value['pnl'])
+ assert value['fees']['funding_paid'] == 99900000
+ assert value['fees']['funding_paid_want'] == 99900000
+ assert value['fees']['borrowing_paid'] == 0
+ # assert value['fees']['borrowing_paid_want'] == 99900000
+ assert value['pnl']['remaining'] == 0
+
+ value = positions.value(2, ctx(PRICE))
+ print(value['fees'])
+ print(value['pnl'])
+ assert value['fees']['funding_paid'] == 0
+ assert value['fees']['funding_received'] == 99900000
+ assert value['fees']['funding_received_want'] == 99900000
+ # assert value['fees']['borrowing_paid'] == 4995000
+ assert value['pnl']['remaining'] == 4995000
+
+ # 3. Another long position is opened again with collateral X.
+ p3 = open(VEL, STX, True, X, 10, price=PRICE, sender=long2)
+ assert not p3.failed
+
+ # 4. Time goes by, equivalent to funding fee of 10%.
+ # Total collateral at this moment is still 2X, so the new total funding paid is 1.2X
+ chain.mine(1)
+
+ print(f"Pool: {pools.lookup(1)}")
+
+ value = positions.value(1, ctx(PRICE))
+ print(f"Long 1: {value['fees']}")
+ print(f"Long 1: {value['pnl']}")
+
+ value = positions.value(3, ctx(PRICE))
+ print(f"Long 2: {value['fees']}")
+ print(f"Long 2: {value['pnl']}")
+
+ assert value['fees']['funding_paid'] == 9990000 #TODO: value with mine(2) is high?
+ assert value['pnl']['remaining'] == 89910000
+
+ print(f"Blocks: {chain.blocks[-1].number - START_BLOCK}")
+
+ # 5. Short position closes and receives funding paid of 1.2X. Quote collateral is now reduced from 2X to 0.8X.
+ tx = close(VEL, STX, 2, price=PRICE, sender=short)
+ print(core.Close.from_receipt(tx)[0]['value'])
+ # fees: [0, 0, 139860000, 139860000, 0, 0, 4995000]
+ assert not tx.failed
+
+ print(f"Blocks: {chain.blocks[-1].number - START_BLOCK}")
+
+ pool = pools.lookup(1)
+ print(f"Pool: {pool}")
+ # assert pool['quote_collateral'] == 9990000 * 0.8 #59940000
+ value = positions.value(3, ctx(PRICE))
+ print(f"Long 2: {value['fees']}")
+ print(f"Long 2: {value['pnl']}")
+ print(f"Long 2: {value['deltas']}")
+
+ # 7. The next long position holder tries to close. They're unable because their collateral is 1x, funding paid is 0.1x.
+ # Collateral calculation is 0.8X + 0.1X - 1X and underflow reverts
+ tx = close(VEL, STX, 3, price=PRICE, sender=long2)
+ print(core.Close.from_receipt(tx)[0]['value'])
+ assert not tx.failed, "close 2nd long"
+
+ # 6. (Optional) Original long closes position. For them funding paid is capped at their collateral,
+ # so funding paid == collateral, so closing does not make a difference on the quote collateral.
+ tx = close(VEL, STX, 1, price=PRICE, sender=long)
+ print(core.Close.from_receipt(tx)[0]['value'])
+ assert not tx.failed, "close original"
+
+ print(f"Blocks: {chain.blocks[-1].number - START_BLOCK}")
+
+ pool = pools.lookup(1)
+ print(f"Pool: {pool}")
+
+
+ assert False
+```
+
+
+
+
+Hence, the issue is indeed valid. About the severity, as I understand, it's indeed high, since there are no extensive limitations, IIUC. Anyone is free to correct me and the POC, but from my perspective it's indeed correct.
+
+But for now, planning to reject @KupiaSecAdmin escalation, accept @spacegliderrrr escalation and upgrade severity to high.
+
+**KupiaSecAdmin**
+
+@WangSecurity - No problem with having this issue as valid but the severity should be Medium at most I think.
+Because, 1) Usually in real-world, funding fees don't exceed the collateral. 2) When positions get into risk, liquidation bots will work most of times.
+
+**WangSecurity**
+
+As far as I understand, this issue can be triggered intentionally, i.e. the first constraint can be reached intentionally, as explained at the end of the Vulnerability Detail section:
+> as a user could abuse it to create a max leverage position which cannot be closed
+
+ But you're correct that it depends on the liquidation bot malfunctioning, which is also mentioned in the report:
+> Eventually the funding fee grows larger than the whole long position (X + Y). it is due liquidation, but due to bot failure is not yet liquidated (which based on comments is expected and possible behaviour)
+
+I agree that this is indeed an external limitation. Planning to reject both escalations and leave the issue as it is.
+
+**WangSecurity**
+
+Result:
+Medium
+Has duplicates
+
+**sherlock-admin3**
+
+Escalations have been resolved successfully!
+
+Escalation status:
+- [KupiaSecAdmin](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/96/#issuecomment-2344287615): rejected
+- [spacegliderrrr](https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/96/#issuecomment-2344359663): rejected
+