ControlledAccessCrowdsale contract #812
Conversation
|
Very into this technique 👍 will do an actual review in the near future |
| * @param _beneficiary Address performing the token purchase | ||
| * @param _sig Access message to provide with purchase. | ||
| */ | ||
| function buyTokens(address _beneficiary, bytes _sig) public payable { |
There was a problem hiding this comment.
giving this contract extends from Crowdsale we will still have the inherited buyTokens public function which can be called to bypass the signature check, am I right?
There was a problem hiding this comment.
Notice that there is no need to override this function, in fact there is comment mentioning no to do that 😛
I think you just need to override the _preValidatePurchase function to use your modifier with msg.data, am I right?
There was a problem hiding this comment.
So, the problem with not overwriting buyTokens(_beneficiary) is that you can't call buyTokens(_beneficiary) with additionnal msg.data. Data will either be ignored or the function will throw for invalid number of arguments (I believe).
What this means is that it would only be possible to buy tokens via the fallback function, where buyer would be msg.sender. With this, msg.data can be the signature and allow the existing fallback and buyTokens functions to be used.
Or, in other words, the following is not possible :
await this.crowdsale.buyTokens(buyer,{ from: delegateBuyer, value: amount, data: signature });Where, in this case, msg.data is 0xFUNCTION000000...BUYERADDRESS and signature is omitted.
There was a problem hiding this comment.
How does the evm handle a data field like
0x + buyTokens function sig + _beneficiary addresss + signature data
? will it call buyTokens with _beneficiary like normal and ignore the signature data part? if so, you could, in the pre-validate, pull the signature from msg.data and validate from there?
| * @param _sig Valid signature from owner | ||
| * @return Validity of access message for a given address. | ||
| */ | ||
| function isValidAccessMessage(address _add, bytes _sig) |
There was a problem hiding this comment.
what about _sender or _address instead of _add?
There was a problem hiding this comment.
can we use ECRecovery here?
| */ | ||
| function () external payable { | ||
| buyTokens(msg.sender, msg.data); | ||
| } |
There was a problem hiding this comment.
following what I mentioned above, I guess there is no need to override the fallback function
| await this.crowdsale.buyTokens(authorized, sig2, { value: value, from: authorized }).should.be.rejected; | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
we may want to extract the access_message and signature calculations to a function or move them to the beforeEach block
|
@PhABC thanks for your PR, this looks like a very nice thing to have! Besides the points raised by @facuspagnuolo, and following your own suggestion: would you consider splitting this in two separate contracts, one SignedWhitelist in ownership, and the actual SignedWhitelistCrowdsale using this? (The naming is up for discussion ;) ) |
|
Sorry about the delay, will make the changes requested! |
| import "../crowdsale/validation/ControlledAccessCrowdsale.sol"; | ||
|
|
||
|
|
||
| contract ControlledAccessCrowdsaleImpl is ControlledAccessCrowdsale { |
There was a problem hiding this comment.
seems like we might not need the mock, since it doesn't do anything beyond ControlledAccessCrowdsale?
There was a problem hiding this comment.
I copied the logic from WhitelistedCrowdsaleImpl.sol. I believe the idea would be that users would create a contract such as contract MyTokenCrowdsale is ControlledAccessCrowdsale { ... } which would specify the parameters fed in the Crowdsale.sol constructor.
There was a problem hiding this comment.
That's what a user would do, for sure, but since this mock doesn't do anything beyond inherit CrontrolledAccessCrowdsale, we can remove it for simplicity. Usually we'd need a mock if we needed to make the constructor payable or something to make testing easier.
There was a problem hiding this comment.
Perhaps you could combine it with a MintableCrowdsale to test the inheritance while we're at it?
|
I'm using this technique for a project and have separated the valid message logic from the crowdsale into a First, it uses You can see what the contracts look like in #883 |
|
Let me know when that's done so I can review this PR! |
|
closing in favor of tracking via #1272 |
🚀 Description
This contract provides a modifier that only allow users possessing an "access message", which has to be provided by the owner of the contract. This is effectively an off-chain whitelist with on-chain verification. More information on this approach can be found on a blog post I wrote on the topic.
I added this contract in the crowdsale/validation directory and added the Crowdsale function calls, but I think access control contract (e.g. whitelists, off-chain messages, etc.) should be separated from crowdsale contracts, since they could be used in contracts other than crowdsale. A
WhitelistedCrowdsalecould easily use aWhitelistcontract, just as with other type of smart contracts (e.g. contracts with access only for small group of users). An independent issue will be made for this latter point (edit : see #813).Note that the fallback function and
buyTokensfunction from the Crowdsale.sol contract have been overwritten/overloaded since a signed message is required.msg.datais used when using the fallback function wheremsg.datais the signed message.npm run lint:all:fix).