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

Staker should not be the Sender #4

Open
florian-bellotti opened this issue Sep 10, 2024 · 7 comments
Open

Staker should not be the Sender #4

florian-bellotti opened this issue Sep 10, 2024 · 7 comments

Comments

@florian-bellotti
Copy link

Description
In the stake function of the Staking contract, the current logic uses get_tx_info().account_contract_address to retrieve the address of the staker. However, the account contract from which a transaction originates is not necessarily the staker, which may lead to incorrect behavior.

Suggested Solution
Instead of relying on account_contract_address, it would be more appropriate to use get_caller_address() or get_execution_info().caller_address to retrieve the address of the staker.

@natan-granit
Copy link

@nagmo-starkware

@nagmo-starkware
Copy link
Collaborator

first, we are planning to change this for another reason (it can be an opening for phishing attacks).
however, I'm not sure I understand:

  1. in which scenario the originating account is not the staker?
  2. assuming the originating account is in fact not the staker how would changing the check to be get_caller_address() solves the problem.

@florian-bellotti
Copy link
Author

When interacting with a contract like an LST (Liquid Staking Token), the process works as follows:
The user sends a transaction to mint tokens. The LST contract then interacts with the delegation pool. In this case, the staker should be the LST contract, not the user who initiated the transaction.

For another scenario, if the user interacts directly with the delegation pool contract while using a paymaster service, the sender is not the user but the paymaster relayer. Here, the paymaster relayer should not be treated as the staker.

Using account_contract_address, which returns the sender's address, instead of get_caller_address, which returns the actual caller's address, can be dangerous and may introduce vulnerabilities.

@nagmo-starkware
Copy link
Collaborator

got it. but both of your examples are of smart contracts interacting with the pool contract where we don't have such checks.
do you have a case where the initiator of the tx is not the staker while actually calling the staking contract?

@florian-bellotti
Copy link
Author

Yes, if the user interacts directly with the staking contract while using a paymaster service, the sender is not the user but the paymaster relayer. Here, the paymaster relayer should not be treated as the staker.

And when initiating an LST contract, the LST will have to call the Staking contract to deploy the delegation pool. The sender will be the LST owner, but the staker should be the LST.

@natan-granit
Copy link

@alon-f ack on SW end. This was also brought up also on internal audits, we are considering exactly what to do. But favoring to enable anyone to enter the address.

@alon-f
Copy link
Collaborator

alon-f commented Oct 21, 2024

We are changing it back to get_caller_address (we are also removing the Operator completely).

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

No branches or pull requests

4 participants