stellar / soroban-examples

Example Soroban Contracts
Apache License 2.0
65 stars 68 forks source link

Add simple token usage example #146

Open tomerweller opened 2 years ago

tomerweller commented 2 years ago

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 commented 2 years ago

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

leighmcculloch commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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