Skip to content

Conversation

@alcueca
Copy link
Contributor

@alcueca alcueca commented Dec 11, 2019

Releasing the simpler version of Issuance.sol. Now we can work on making the other one fancier.

@alcueca alcueca requested review from uivlis and vibern0 December 11, 2019 22:34
@uivlis
Copy link
Contributor

uivlis commented Dec 12, 2019

Should we have using SafeERC20 for ERC20 inside Issuance for the transfer and transferFrom functions?

@alcueca
Copy link
Contributor Author

alcueca commented Dec 12, 2019

This is an interesting article about SafeERC20: https://decentraland.org/blog/technology/safe-erc20-transfers.

As far as I understand the purpose of SafeERC20 is to protect against the fact that different implementations of ERC20 return different values when a transfer fails (mostly because the amount to transfer is higher than the balance or than the allowance).

We have two calls to transfer in Issuance.sol. One for transferFunds and one for cancelInvestment.

In the case of transferFunds if the transfer call fails it makes no difference whether there is a revert or not. State doesn't change either way.

In the case of cancelInvestment it is trickier. Using an ERC20 that doesn't revert on failed transfers as a currency token would erase investments without transferring the currency if the contract balance drops to zero. For that to happen there should be something really wrong happening, though, since the sum of all investments must always be lower or equal than the contract balance of currency token. That is, unless someone can extract currency token from the contract without touching the investments, and there we would have a more serious problem.

So thanks for the suggestion, but I'm not sure SafeERC20.sol adds much value here. If you know of a use case where SafeERC20 is really useful I'd be interested.

@alcueca alcueca merged commit 0140134 into dev Dec 12, 2019
@alcueca alcueca deleted the release/issuance branch December 12, 2019 11:11
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

Successfully merging this pull request may close these issues.

3 participants