Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra deposit and withdraw methods #150

Merged
merged 21 commits into from
Nov 7, 2024

Conversation

faurdent
Copy link
Collaborator

No description provided.

@faurdent faurdent self-assigned this Oct 31, 2024
@faurdent faurdent marked this pull request as ready for review October 31, 2024 09:46
@djeck1432 djeck1432 requested a review from tensojka November 4, 2024 18:18
Copy link
Collaborator

@tensojka tensojka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a few things to fix

src/deposit.cairo Outdated Show resolved Hide resolved
src/deposit.cairo Show resolved Hide resolved
@@ -133,8 +155,8 @@ mod Deposit {
ekubo_limits: EkuboSlippageLimits,
pool_price: TokenPrice
) {
let user_acount = get_tx_info().unbox().account_contract_address;
assert(user_acount == self.owner.read(), 'Caller is not the owner');
let user_account = get_tx_info().unbox().account_contract_address;
Copy link

@9oelM 9oelM Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are using let user_account = get_tx_info().unbox().account_contract_address; here and in close_position, as opposed to using get_caller_address() in withdraw and extra_deposit. I was wondering if there is a special reason for this?

Ideally i think it should be replaced by get_caller_address(). You can read about the security implication at:
https://github.com/starknet-edu/starknetbook/blob/8cfcd33ccd9afee425f31a10a721f20a84554ca5/src/ch02-14-security-considerations.md?plain=1#L150.

But in a nutshell, something like this could happen:

  1. Alice deploys her own Spotnet contract, and she is the owner
  2. Bob deploys another contract (let's call it 'Evil') that calls loop_liquidity or close_position of Alice's Spotnet contract
  3. Bob somehow tricks Alice into calling a function in Evil. Alice ends up unintentionally calling loop_liquidity or close_position not necessarily knowing so.

This is possible because you are checking the address of the tx's origin, not the caller address.

Granted this is not a big problem because Bob can't steal any money, but Bob might be able to cause Alice to lose money in certain cases, obviously. Probably some easiest way would be closing a premature position that hasn't profited or sending along a wrong price.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was an issue with testing. If there is a way we can bypass this behavior it would be great to use get_caller_address everywhere, but I didn't find any info on that. So get_caller_address is used when we are not swapping through Ekubo.

src/deposit.cairo Outdated Show resolved Hide resolved
src/deposit.cairo Outdated Show resolved Hide resolved
@faurdent faurdent requested review from tensojka and 9oelM November 6, 2024 17:38
@djeck1432 djeck1432 merged commit 9fa84d1 into main Nov 7, 2024
4 checks passed
@djeck1432 djeck1432 deleted the feat/add-extra-deposit-withdraw branch November 7, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants