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 simple token usage example #146

Open
tomerweller opened this issue Oct 9, 2022 · 7 comments
Open

Add simple token usage example #146

tomerweller opened this issue Oct 9, 2022 · 7 comments

Comments

@tomerweller
Copy link
Contributor

tomerweller commented Oct 9, 2022

Our current examples that make use of tokens cover a lot of ground. Single offer, Liquidity pool and Timelock are all great but are not trivial and also implement advanced auth.

It would be beneficial to have an almost trivial contract example accompanied by a tutorial that focuses on basic token usage.

One option is to make a simplified version of the timelock contract: single claimant, only "after" timebound, no advance auth.

@tomerweller
Copy link
Contributor Author

@dmkozh, as the authoer of timelock, is simplifying it a low hanging fruit?

@leighmcculloch
Copy link
Member

leighmcculloch commented Oct 9, 2022

I think we could remove the use of soroban-auth from timelock, because it takes on a separate call to token approve anyway.

It should be a small lift of replacing verification with env.invoker() and replacing soroban_auth::Identifier with soroban_sdk::Address.

@dmkozh
Copy link
Contributor

dmkozh commented Oct 10, 2022

Sure, I could do the change. This example has been written a while ago, hence auth is tricky. Just to double-check, the proposal would be to get rid of the signatures completely, so that we could just use the invoker and not require an advance approve call, right?

I also wanted to cover some non-trivial argument types with this example (vec/enum), hence I'm not sure about simplifying the 'claimable balance' part - it's just a couple lines of code that aren't too complex IMHO. Alternatively, we could just rename this contract to something like 'token_transfer` and simplify as much as possible. 'timelock' is not the first place I would look at when searching an example of token usage.

@dmkozh
Copy link
Contributor

dmkozh commented Oct 10, 2022

FWIW for token_transfer we could get rid of the time lock too, just have deposit to contract and define a single claimant, then in claim just check for the claimant only. This way the example would be focused on transfers too/from contract only.

@leighmcculloch
Copy link
Member

so that we could just use the invoker and not require an advance approve call, right?

You'd still need the advanced approve call I think.

The rest of the example, unrelated to auth, seems fine to me.

@dmkozh
Copy link
Contributor

dmkozh commented Oct 10, 2022

You'd still need the advanced approve call I think.

Oh, true, that makes me think that we should probably consider passing the source account recursively to all the sub-contract calls (unless there is some security risk, but I don't see it).

@leighmcculloch
Copy link
Member

we should probably consider passing the source account recursively to all the sub-contract calls (unless there is some security risk, but I don't see it)

This would open up a significant security footgun opportunity: relay attacks. This is where a message sent to contract A can be relayed to contract B, even if the invoker didn't intend.

Ethereum provides a way to find out the original invoker via the tx.origin, but it should really never be used in authorization. E.g. https://hackernoon.com/hacking-solidity-contracts-using-txorigin-for-authorization-are-vulnerable-to-phishing

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
@leighmcculloch @tomerweller @dmkozh and others