Decouple RefundVault from RefundableCrowdsale

[brunobar79]

🚀 Description

After a quick chat with @spalladino and @fiiiu we all agreed that it would be a good idea to decouple RefundVault from RefundableCrowdsale.

Right now, after the Crowdsale goal is reached, ALL the funds are transferred to the wallet on finalization and there's no way of adding / overriding any logic.

After this, you should be able to create a contract that extends from RefundVault which will give you the ability to override its methods ( mainly close() and refund() )

• 📘 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).

[spalladino]

Let's use the same params order (vault, token or token, vault) for both the mock here and the SampleCrowdsale example, just for the sake of consistency.

[brunobar79]

@spalladino 100% agree. Just pushed a fix

[spalladino]

LGTM!

[fiiiu]

Looks good!

[fiiiu]

@brunobar79 thanks for your work! I'm closing this as a refactoring of the crowdsale codebase is underway, your changes are included in the new version, please see #744.

[martriay]

Actually I find this change a bit dangerous.

If it's not the crowdsale who deploys the RefundVault, it's not guaranteed that the crowdsale is going to be its owner. This requires trust in the deployer that it will set the crowdsale as the vault owner, otherwise it can steal all the funds anytime by providing a modified RefundVault version where anyone (not only the owner) can call RefundVault.deposit, allowing the crowdsale to send its funds to be stolen.

I understand that this change enables the possibility of using a modified version of RefundVault and it also lowers deployment costs, to mitigate the issue maybe the RefundVault can be added in a post-construction stage where the crowdsale can know its own address to check if it's the vault's owner.

Thoughts?

[spalladino]

I wasn't aware that you could not check `this` in the constructor to add that verification. Still, I believe it's good to keep the vault decoupled, especially because it's in the direction needed for setting it up in an on-chain factory on zOS. Maybe we can think of other checks?

…

Sent from my mobile

On Feb 19, 2018 11:23, "Martín Triay" ***@***.***> wrote: Actually I find this change a bit dangerous. If it's not the crowdsale who deploys the RefundVault, it's not guaranteed that the crowdsale is going to be its owner. This requires trust in the deployer that it will set the crowdsale as the vault owner, otherwise it can steal all the funds anytime by providing a modified RefundVault version where anyone (not only the owner) can call RefundVault.deposit, allowing the crowdsale to send its funds to be stolen. I understand that this change enables the possibility of using a modified version of RefundVault and it also lowers deployment costs, to mitigate the issue maybe the RefundVault can be added in a post-construction stage where the crowdsale can know its own address to check if it's the vault's owner. Thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#734 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaOJAKv3tp032gUoyzNozmFIpUvO0-Yks5tWbwYgaJpZM4SDa4j> .

[martriay]

You're right, I thought that a constructor couldn't know its own address, but it can. I suggest performing that check then.

[spalladino]

Sounds good. I'm wondering if we want to enforce that refund vaults are Ownable though, or we may want to enable other options. However, for the time being, I think we're perfectly fine with this check.

…

Sent from my mobile

On Feb 19, 2018 11:36, "Martín Triay" ***@***.***> wrote: You're right, I thought that a constructor couldn't know its own address, but it can. I suggest performing that check then. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#734 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaOJNjoDhc55m5Ol5MyuotVg9gdFU9Mks5tWb8QgaJpZM4SDa4j> .

[martriay]

@spalladino If by "we're perfectly fine with" you mean "this won't work even if you can know your address at construction time because how can the RefundVault know it before that? we should forget about" (this check), then we agree.

Whoops

[spalladino]

Good point. Still, even if we skip that check, any purchase to the crowdsale would later fail since deposit is only callable by the owner. Again, I'd keep this implementation, considering it's in the right direction w.r.t. zOS on-chain factories.

[i6mi6]

So as far as I understand correctly RefundableCrowdsale doesn't function properly, because deposit() is marked as ownerOnly in RefundVault?

[spalladino]

@i6mi6 it does, you only have to remember to transfer ownership to the crowdsale contract before starting the sale.

[i6mi6]

Apologies for the noob-ish question but doesn't deposit() function invocation happen by an investor who is not the owner of the contract? Who is going the be msg.sender at the time of deposit invocation?

[martriay]

@i6mi6 the crowdsale is supposed to be the owner of the RefundVault contract, and its _fowardFunds() is the one that performs the call to deposit. Don't confuse msg.sender, which stands for the caller of the function, with tx.origin, the address that initated the transaction.