Tests and contract rework for audit #69

faurdent · 2024-10-22T21:49:21Z

No description provided.

Dashboard-API-fixing

dashboard backendUrl update

Added plug to dashboard in case of missing API data

…and made draft tests

tensojka

How is decided through which Ekubo pools you'll be routing? (Did you look into that?)

tensojka · 2024-10-28T08:20:50Z

src/deposit.cairo

+        /// * `debt_token`: ContractAddress - Address of the token used as borrowing.
+        /// * `pool_key`: PoolKey - Ekubo type for obtaining info about the pool and swapping
+        /// tokens.
+        /// * `supply_price`: felt252 - Price of `supply` token in terms of `debt` token in Ekubo


Is it exactly just price? How is that expressed? It must be a fraction or fixed point number right; what kind is it? (edit: later found in docs)

And – can you give an example where supply_price is > 2**128? (I don't think it can reach that high?)

This format is used in Ekubo? (I don't think so)

I don't think it makes sense to have the price be in relation to one "full" (human-readable) unit; then you need to fiddle with decimals.

So 2 questions:

Why do you need the price? (Future slippage calculation?)

Don't you get the price from Ekubo after executing a swap?

Why aren't you using the same format as Ekubo (since that's where you interact with the price) for storing the price? (You will then be able to truly get rid of decimals:) (I understand the Ekubo math is very hard and it might very well be a good reason)

Calculation of how much can I borrow from zkLend, we will borrow 80%(I will try and test lower value) of allowed amount so precise median price from the oracles as used in zkLend is not required I suppose.

It's not given exactly on swap. However I can get pool price from Ekubo as sqrt_ratio and then convert to the normal price. I am concerned about price of base token in terms of quote token(so ETH amount for USDC in ETH/USDC) and I wanted to calculate it on backend with normal decimals, but I should be able to handle it to depend on external params less.

Ekubo using prices as ticks or sqrt_ratio and zkLend as normal amounts, so there is a need for conversions unfortunately.

tensojka · 2024-10-28T08:21:59Z

src/deposit.cairo

+            let SQRT_LIMIT_REPAY = if supply_token == pool_key.token0 {
+                18446748437148339061
            } else {
-                (6277100250585753475930931601400621808602321654880405518632, 18446748437148339061)
+                6277100250585753475930931601400621808602321654880405518632


This is essentially the max slippage no? Is it correct to hardcode it?

I think I will calculate 3-5% slippage from current sqrt_ratio

tensojka · 2024-10-28T08:30:34Z

src/deposit.cairo

+            assert(
+                amount * usdc_price / deposit_token_decimals.into() >= 1000000,
+                'Loop amount is too small'
+            ); // User needs to have at least 1 USDC, so rounded down amount is OK.


You are only using usdc_price to calculate whether the loan amount is at least 1 USDC.

Why is this limitation in place?

This does not actually check anything because the caller can spoof the usdc_price. (!!)

If this is really necessary (and I don't think it is), you can adjust the usdc_price format such that you don't need to fetch decimals! (Basically embed the decimals there and pass only the ratio)

usdc_price is actually not really price of USDC, is it?

I added it because sometimes for small amounts depositing and borrowing is failing because of number of iterations(or steps made by program). I can remove it but sometimes transaction will fail with unclear message(same will be if user or our frontend inputs wrong price). I had some doubts when adding this because of the same reason, so I will remove it and try to locate the reason and location of fail, it's number of iterations I would consider limiting it instead(number of deposits and borrowings in a loop).

tensojka · 2024-10-28T08:31:41Z

src/deposit.cairo

        ) {
-            assert(get_caller_address() == self.owner.read(), 'Caller is not an owner');
+            // TODO: Add borrow factor
+            let user_acount = get_tx_info().unbox().account_contract_address;


Why not get_caller_address() ? (Maybe it was removed in a new Cairo version that I'm not yet aware of?)

It was made because of testing troubles. In a contract, when swapping we are getting callback from Ekubo. When using normal cheat_caller_address, it cheats all input calls to the contract, and code fails with 'Caller is not the core' error from Ekubo core contract, so I cheat the account that initiated transaction instead(practically it should be the caller themself). However I agree this functionality is an unusual and unpredicted, so I should include it in the docs.

I think okay to just add an explainer comment like this to the code. Interesting!

tensojka · 2024-10-28T08:49:06Z

src/deposit.cairo

-                is_token1: !is_token1_repay_swap,
-                sqrt_ratio_limit: SQRT_LIMIT_WITHDRAW,
-                skip_ahead: 0
+                debt = zk_market.get_user_debt_for_token(contract_address, debt_token).into();


If you previously called repay_all (line 311), this will be zero, no? Either remove or assert that it's zero in that case.

feat: function claim_rewards for claiming DeFi Spring rewards

tensojka · 2024-10-30T14:56:15Z

docs/spotnet.md

+* `supply_price`: TokenPrice - Price of `supply` token in terms of `debt` token in Ekubo pool(so for ex. 2400000000 USDC for ETH).
+* `debt_price`: TokenPrice - Price of `debt` token in terms of `supply` token in Ekubo pool(for ex. 410000000000000 ETH for USDC).


Is this really the price in Ekubo pool?

tensojka · 2024-10-30T14:57:28Z

src/deposit.cairo

-        let amount_base_token = token_price * borrow_capacity;
-        let amount_quote_token = amount_base_token.into() / decimals_difference.into();
-        ((amount_quote_token - total_borrowed.into()) / 100_u256 * borrow_const).try_into().unwrap()
+        let borrow_const = 60;


This depends on the asset, no? It can change as zkLend changes its risk parameters I think?

But maybe the only thing this will cause is that it will loop more times than necessary?

zkLend risk parameters are borrow and collateral factors and we are getting them from their contract. This one is how much of available amount we are borrowing(approximately 60% now), while causing looping more times, lower value will increase health factor. I can add it to the constructor so we'll be able to change it dynamically without changing the code

Why should that matter? Why can't we be borrowing 99 %? The health factor is decided by the leverage at the end only.

tensojka · 2024-10-30T14:58:04Z

src/deposit.cairo

+            let curr_contract_address = get_contract_address();
+            assert(
+                token_dispatcher.allowance(user_acount, curr_contract_address) >= amount,
+                'Approved amount incuficient'


tensojka · 2024-10-30T15:00:14Z

src/deposit.cairo

-            let mut deposited = amount;
-            let mut total_borrowed = 0;
-            let mut accumulated = 0;
+            let (mut total_borrowed, mut accumulated, mut deposited) = (0, 0, amount);


I would just keep this on three separate lines

tensojka · 2024-10-30T15:08:34Z

src/types.cairo

 use starknet::ContractAddress;

-pub type TokenPrice = u256;
+pub type TokenPrice = u128;
 pub type TokenAmount = u256;
 pub type Decimals = u64;


You don't actually store Decimals in this data type, you store 10**x where x is the decimal count, which is unclear

tensojka · 2024-10-30T15:09:33Z

src/deposit.cairo

+            proof: Span<felt252>,
+            airdrop_addr: ContractAddress
+        ) {
+            assert(self.is_position_open.read(), 'Open position not exists');


What if the position was closed in the meantime? Why do we need this check?

tensojka · 2024-10-30T15:10:00Z

src/deposit.cairo

+            airdrop_addr: ContractAddress
+        ) {
+            assert(self.is_position_open.read(), 'Open position not exists');
+            assert(proof.len() != 0, 'Proof Span cannot be empty');


I appreciate the defensiveness, but this is not necessary; it will fail when calling claim() anyway

faurdent added 12 commits October 20, 2024 13:21

Swap method moved to internal trait

b3a9e01

Docstrings added, swap removed from public interface

d994326

New assertions

97e9c24

Pragma added for testing

f69b44c

More validations for contract stability. Owner check reworked

7349a40

First fuzz test for looping

3660f96

New tests for liquidity loop

bb8c80b

Dependencies updated

f331bce

Use input amount computing on Ekubo to not loose token on last swap

8fd1a54

Closing position after time passed test case

81c9172

Type aliases added

ef1f2f3

Testing amounts. Investigating liquidation.

704c8d7

faurdent marked this pull request as ready for review October 24, 2024 11:35

faurdent mentioned this pull request Oct 24, 2024

Add test cases for smart contract #83

Closed

AndriiBogomolov and others added 16 commits October 26, 2024 19:58

dashboard-API-fixing

b72e56a

Dashboard-API-fixing

dashboard backendUrl update

28ae2cc

dashboard backendUrl update

feat/added plug to dashboard

138ca25

Added plug to dashboard in case of missing API data

added missing response models

25c58e4

fixed alembic

de7c7a0

Created tests direcotry in web_app, created test_user.py file inside …

619a174

…and made draft tests

Fixed draft tests

8bbd9bd

Created pytest.ini file

cf45f81

Fixed draft tests

b2cd1c4

Removed print debuger

8423f7d

Created working tests

ff59022

Added pytest, pytest-asyncio and httpx

437deb9

Replaced response.status_code == 200 to response.is_success

f446cd0

merge with audit-preparation

ff16906

dependency error fix

7765235

fix conflicts

d05c6b6

SoarinSkySagar added 7 commits October 26, 2024 21:27

fix conflict

c9ab4c3

fix

b2db2a7

fix conflict

9c5f9dd

merge conflict

06c1ed4

added tests

0b68f30

minor changes

a4efff1

additional assert to check reward amount

7db97d3

tensojka reviewed Oct 28, 2024

View reviewed changes

djeck1432 assigned faurdent Oct 28, 2024

SoarinSkySagar and others added 8 commits October 29, 2024 00:04

requested changes

ad02dd9

scarb fmt

c9cedfd

Merge pull request #118 from SoarinSkySagar/sync

351caf8

feat: function claim_rewards for claiming DeFi Spring rewards

Ekubo slippage parameter added

8b45606

Break added to not add debt

9dce53a

Docs updated

412326e

Proper types set. Imports optimized

2fd5ee3

Add explanation on owner check in docs

d691ec5

faurdent requested a review from tensojka October 29, 2024 21:44

tensojka reviewed Oct 30, 2024

View reviewed changes

Renamings, docs changes

259e497

faurdent requested a review from tensojka October 30, 2024 16:56

Borrow const moved to Deposit Data. Docs and tests updated

0b7f0af

djeck1432 merged commit 18bd439 into main Nov 1, 2024
3 of 6 checks passed

djeck1432 deleted the fix/audit-preparation branch November 1, 2024 09:57

faurdent mentioned this pull request Nov 6, 2024

Add extra deposit and withdraw methods #150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests and contract rework for audit #69

Tests and contract rework for audit #69

faurdent commented Oct 22, 2024

tensojka left a comment

tensojka Oct 28, 2024 •

edited

Loading

tensojka Oct 28, 2024

tensojka Oct 28, 2024

faurdent Oct 28, 2024

tensojka Oct 28, 2024

faurdent Oct 28, 2024

tensojka Oct 28, 2024

faurdent Oct 28, 2024

tensojka Oct 28, 2024

faurdent Oct 28, 2024

tensojka Oct 29, 2024

tensojka Oct 28, 2024

tensojka Oct 30, 2024

tensojka Oct 30, 2024

faurdent Oct 30, 2024

tensojka Oct 30, 2024

tensojka Oct 30, 2024

tensojka Oct 30, 2024

tensojka Oct 30, 2024

tensojka Oct 30, 2024

tensojka Oct 30, 2024

		* `supply_price`: TokenPrice - Price of `supply` token in terms of `debt` token in Ekubo pool(so for ex. 2400000000 USDC for ETH).
		* `debt_price`: TokenPrice - Price of `debt` token in terms of `supply` token in Ekubo pool(for ex. 410000000000000 ETH for USDC).

Tests and contract rework for audit #69

Tests and contract rework for audit #69

Conversation

faurdent commented Oct 22, 2024

tensojka left a comment

Choose a reason for hiding this comment

tensojka Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tensojka Oct 28, 2024 •

edited

Loading