starkware-libs / starknet-staking

starknet-staking
Apache License 2.0
36 stars 10 forks source link

Staker should not be the Sender #4

Open florian-bellotti opened 1 week ago

florian-bellotti commented 1 week ago

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 onaccount_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 commented 2 days ago

@nagmo-starkware

nagmo-starkware commented 1 day ago

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 commented 14 hours ago

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 commented 14 hours ago

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 commented 14 hours ago

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.