fix: force RefundableCrowdsale to also be a PostDeliveryCrowdsale

[shrugs]

Fixes #877

🚀 Description

When participating in a refundable crowdsale, because tokens were delivered immediately, malicious investors could previously sell tokens to unsuspecting users and then collect their own ether-based refund after the goal is not met.

This change forces RefundableCrowdsale to also be a PostDeliveryCrowdsale where users are only allowed to withdraw tokens if the goal is reached, the crowdsale is over, and it is finalized.

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

[shrugs]

Any reason why ci is reporting failure? logs look happy

[frangio]

It's the linting:

examples/SampleCrowdsale.test.js
  69:11  error  'expectedTokenAmount' is assigned a value but never used  no-unused-vars

I've been confused by this before. Maybe linting should run as a separate job?

[shrugs]

nice, thanks; looks like I didn't actually run that link command like the checkbox says I did 😅

[frangio]

The smart contracts look good. I'm not convinced by the way the PostDelivery behavior was implemented, parameterizing by the endCrowdsale procedure.

I left some concrete ideas.

[frangio]

I think I prefer having this in a before block as it was before if that's all that's strictly necessary.

Actually, can you check if removing this line makes the tests fail? I feel like it was a Ganache bug that may have been fixed by now.

[frangio]

There are a couple issues here.

endCrowdsale is actually a function with no arguments which wraps each test's this.endCrowdsale. So, these parameters (investor, etc.) are never used. I think it's reasonable to expect the endCrowdsale function to not need the parameters because the behavior itself receives them from there.

I don't think it's necessary or okay to receive endCrowdsale as a behavior argument; the behavior should just access this.endCrowdsale directly. Refundable's has a boolean parameter but it should have an appropriate default value so that this works, which should be true in this case.

[frangio]

lessThanGoal was meant to be used "by itself", not together with another investment of value, so it's kind of a coincidence that value + lessThanGoal < goal.

Maybe don't do this second investment at all if goalReached == true?

[frangio]

What about removing this.endCrowdsale altogether and using goal instead of value here? I think this communicates the idea that: this should behave exactly like a PostDeliveryCrowdsale as long as the money invested is at least goal. It's not ideal because it wouldn't be testing a scenario where the goal is reached by more than one investment. But I'm not convinced by this.endCrowdsale.

[vittominacori]

@frangio @shrugs
You are forcing investors to make another transaction after the crowdsale end to claim their tokens.

If the issue is that users can transfer their tokens before the end, is it not better to force using an ERC20 token with a RBAC on who is enabled to transfer?

For instance adding a transferEnabled boolean to the token and add a specific role for transfer events.
Then add the canTransfer modifier to transfer and transferFrom.

modifier canTransfer() {
    require(transferEnabled || hasRole(msg.sender, ROLE_TRANSFER));
    _;
  }

I can build this token.
What do you think about?

[shrugs]

That's another fine approach. The difference is that with a post delivery crowdsale the logic is in the crowdsale and not the token contract. @frangio which approach do you like more?

[vittominacori]

My concern with forcing a post delivery is that investors should be able to make a transaction to the withdrawTokensfunction.

If buyTokens could be simple like sending an amount of ETH, to withdraw their tokens, instead, they should be able to copy the ABI, use MEW... or the crowdsale builder should prepare a DApp where they can withdraw tokens with MetaMask (and investors should have invested using MetaMask or import private keys into).

So, if it is not strictly necessary (as developer and I think also as business developer), I would prefer to build the logic when I build the token and not to force people to understand how to use ABIs.

[nventuro]

Closing in favor of #1543: this PR got stale, and it contains lots of unrelated changes (e.g. code formatting, SampleCrowdsale tests, etc.).

Note that the actual fix is the same as in #1543 though.