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

Pay-direct-to-smart-contract risks using fallback function #53

Open
rstormsf opened this issue Aug 22, 2017 · 13 comments
Open

Pay-direct-to-smart-contract risks using fallback function #53

rstormsf opened this issue Aug 22, 2017 · 13 comments

Comments

@rstormsf
Copy link
Contributor

rstormsf commented Aug 22, 2017

As an investor, I'd like to buy tokens using contact address which is fallback function.

I see this pattern is used quite extensively in your contracts with 1 caution: all those contracts have to throw an exception in fallback.

  function isCrowdsale() public constant returns (bool) {
    return true;
  }

If I unlock the fallback function and provide some default behaviour, then it would break those checks because of strange behaviour by design in solidity, if a method does not exist it will instead execute the fallback function, and if the fallback function does not raise an exception it will return 1 causing the check to pass.. I guess what could be done instead is using some kind of magic constant, e.g.

contract Crowdsale {
   uint256 public constant CROWDSALE_MAGIC = 0x1234;
}

contract Token {
  address crowdsale;
  function doSomething(address _a) {
    require(Crowdsale(_a).CROWDSALE_MAGIC() == 0x1234);
    crowdsale = _a;
  }

what do you think?

@miohtama
Copy link
Contributor

miohtama commented Aug 23, 2017

As a token sale hosting expert, I highly recommend against not enabling the fallback function.

People will send payments directly from exchange wallets where data field is not provided, e.g. Coinbase. People will no be able to receive tokens to these addresses. Coinbase customer support will refuse to help them.

It creates a customer support and PR nightmare.

If you understand this significant risk that makes users to lose their tokens, we can discuss.

@rstormsf
Copy link
Contributor Author

rstormsf commented Aug 23, 2017

Got it. Makes sense. Thank you for your explanation. So, how do you recommend people to do a crowdsale? Do you provide them an address with Data (bytecode) to paste in ? Or just use front end page that talks to metamask/parity ?
or both.

@miohtama
Copy link
Contributor

Hi @rstormsf

You can see TokenMarket active widget on here:

https://rivetzintl.com/sale

and

here https://reality-clash.com/ico/sale.php

It contains example how to make the payment process.

@rstormsf
Copy link
Contributor Author

rstormsf commented Sep 4, 2017

@miohtama I'm very familiar with the process. Thank you for your thoughts. After careful consideration, I think disabling fallbacks is an actually decent idea.

@miohtama
Copy link
Contributor

miohtama commented Sep 4, 2017

All decent token sales have been doing this since Golem (Nov 2016). People still do not always understand Data field, because exchange transactions do not this give this option. However education is slowly kicking in and I am starting to see more and more participants who get the transaction right on the first try and they do not try to force it out from Coinbase.

@rstormsf
Copy link
Contributor Author

rstormsf commented Sep 13, 2017

I agree. Unfortunately, some of the clients still want to accept the payment(#greed) to not confuse people with data field ;-)
The other problem that I faced with an incorrect gas limit that people don't put into.
Metamask/Parity not always estimates the correct amount of gasLimit.

@rstormsf
Copy link
Contributor Author

rstormsf commented Sep 30, 2017

https://github.com/KyberNetwork/TokenDistributionContracts/blob/6b04dbf730ffc55c3d2850969dbbfb89e69dfcfc/TokenSale/contracts/KyberNetworkTokenSale.sol#L62
@miohtama what do you think of that?
I really like this idea to make the game fair.

require( tx.gasprice <= 50000000000 wei );

@miohtama
Copy link
Contributor

miohtama commented Oct 1, 2017

@rstormsf This is alternative approach to do it.

@miohtama miohtama reopened this Oct 1, 2017
@miohtama
Copy link
Contributor

miohtama commented Oct 1, 2017

The gas limit estimation problem is usually not a problem, as often it overestimates the gas limit and rarely underestimates.

@rstormsf
Copy link
Contributor Author

rstormsf commented Oct 2, 2017

you really didn't read my message. @miohtama who said anything about gasEstimate.
I was talking about gasPrice to prevent whales to get ahead of the line in pending tx pool

@miohtama
Copy link
Contributor

A victim use case from the wild: https://ethereum.stackexchange.com/a/34558/620

@miohtama
Copy link
Contributor

@rstormsf I re-read your comment and now understand. This is a good practice, though might not work in the contemporary anymore in the hard coded manner. Ethereum gas fees tend to swing a lot nowadays so setting an upper limit in non-careful manner might be shooting yourself in the foot.

@miohtama miohtama changed the title Fallback problem Fallback problem - or pay-direct-to-smart-contract risks Feb 28, 2018
@miohtama miohtama changed the title Fallback problem - or pay-direct-to-smart-contract risks Pay-direct-to-smart-contract risks using fallback function Feb 28, 2018
@miohtama
Copy link
Contributor

From the UX point of view, direct payment addresses are no longer needed as wallets have have better and better support for Web3.js and transaction payloads.

For example you can have single click checkout with MetaMask and web3.js:

https://tokenmarket.net/what-is/how-to-buy-into-a-token-sale-with-metamask-wallet/

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

2 participants