search

Concordium / concordium-rust-smart-contracts

Libraries and tools for writing and testing smart contracts on Concordium
https://docs.rs/concordium-std/latest/concordium_std/
Mozilla Public License 2.0
57 stars 35 forks source link

tx.origin/invoker variable should not be used for authentication in smart contracts #110

Closed DOBEN closed 2 years ago

DOBEN commented 2 years ago

Description Solidity uses the variable tx.origin (the equivalent variable is invoker on the Concordium chain) to denote the address that initiated a tx.

Smart contract best practice guides advise NOT to use tx.origin/invoker for authentication/authorization in smart contracts because phishing attacks are possible with such a setup. Additional reading here and here

Suggested changes to following line: claim_eq!(ctx.owner(), ctx.sender());

Alternative suggestion: I know that these smart contracts are not meant for production but I suspect new smart contract developers to kick start their projects based on these examples nonetheless. If we want to keep the invoker variable at this line then I would at least add a warning comment above it so people are aware of it.

Please remove the label "question" if it is ok that I work on it or close the issue to reject the suggested changes.

abizjak commented 2 years ago

Makes sense. There is another bug in that contract, which is the use of claim_eq. That is only meant for testing, it's like an assert.

In contract entrypoints one should use ensure or ensure_eq instead.

abizjak commented 2 years ago

Please just make a PR fixing this.

Edit: And by fixing this I mean using sender instead of invoker. So

    ensure!(ctx.sender().matches_account(&ctx.owner()));