feat: Add SafeERC20 #469

sohkai · 2018-12-17T09:46:23Z

Adds a SafeERC20 library that should be used to enhance ERC20 types. The safeTransfer(), safeTransferFrom(), and safeApprove() methods ensure standards-compliance (e.g. always returning a value) with different token interfaces in the wild.

Note that the change for VaultRecoverable.sol means every AragonApp is affected.

Bytecode comparison:

                               CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                       49200 more gas       +246               0
APMRegistry.json               50600 more gas       +253               0
AppStub.json                   49000 more gas       +245               0
AppStub2.json                  49200 more gas       +246               0
AppStubConditionalRecovery.js… 49200 more gas       +246               0
AppStubDepositable.json        49200 more gas       +246               0
AragonApp.json                 49200 more gas       +246               0
ENSSubdomainRegistrar.json     47600 more gas       +238               0
EVMScriptRegistry.json         47600 more gas       +238               0
EVMScriptRegistryFactory.json  Same                 0                  +238
Kernel.json                    48600 more gas       +243               0
KernelConstantsMock.json       48600 more gas       +243               0
KernelDepositableMock.json     48600 more gas       +243               0
KernelPinnedStorageMock.json   48600 more gas       +243               0
KernelSetAppMock.json          48600 more gas       +243               0
MockScriptExecutorApp.json     47800 more gas       +239               0
Repo.json                      49000 more gas       +245               0
TestACLInterpreter.json        49200 more gas       +246               0
TokenMock.json                 202400 more gas      +1012              +13
UnsafeAppStub.json             49000 more gas       +245               0
UnsafeAppStubDepositable.json  49200 more gas       +246               0
UnsafeAragonApp.json           49200 more gas       +246               0
UnsafeAragonAppMock.json       49200 more gas       +246               0
UnsafeRepo.json                49000 more gas       +245               0
UpgradedKernel.json            48600 more gas       +243               0
VaultMock.json                 49200 more gas       +246               0

Warning: could not compare some contracts that might have been renamed or removed
  Missing from /Users/brett/Development/aragon/aragonOS/bytecode: GeneralERC20.json, SafeERC20.json, SafeERC20Mock.json, TokenReturnFalseMock.json, TokenReturnMissingMock.json

sohkai · 2018-12-17T09:51:39Z

contracts/common/VaultRecoverable.sol

-            ERC20(_token).transfer(vault, amount);
+            ERC20 token = ERC20(_token);
+            uint256 amount = token.balanceOf(this);
+            require(ERC20(_token).safeTransfer(vault, amount), ERROR_TOKEN_TRANSFER_FAILED);


Not sure if we should add this change at this moment—making sure the token transfer was successful is the right thing to do, but it's really only a problem for non-transferable tokens.

I think it's fine this way.

coveralls · 2018-12-17T10:13:16Z

Coverage increased (+0.006%) to 98.767% when pulling b26941b on safe-erc20 into c4e0fb1 on dev.

bingen

Nice job!

bingen · 2018-12-17T18:26:10Z

contracts/common/SafeERC20.sol

+
+// Same as ERC20 except the return values of these interfaces have been removed to allow us to
+// manually check them
+interface GeneralERC20 {


I would call it BrokenERC20 ;-)

I propose NotERC20 or ANERC20 ('accidentally not ERC20')

contracts/common/VaultRecoverable.sol

contracts/test/mocks/SafeERC20Mock.sol

izqui · 2018-12-17T20:41:10Z

contracts/common/SafeERC20.sol

+            }
+
+            // Not sure what was returned: don't mark as success
+            default { }


Really nice assembly 👏

izqui · 2018-12-17T20:51:54Z

Will review in detail closer to releasing this

izqui

👏👏👏👏👏

izqui · 2019-01-03T09:39:22Z

contracts/common/SafeERC20.sol

+            // 32 bytes returned: check if non-zero
+            case 0x20 {
+                // Copy 32 bytes into scratch space
+                returndatacopy(0x0, 0x0, 0x20)


I like how by doing this optimization we save a few mload and mstore. Given that we read this memory just immediately after we should be okey using this memory address, but I always feel a little uneasy that something else may be counting on this memory space not changing.

Something else may be counting on this memory space not changing

I really hope not... 0x00 - 0x3f is scratch space for doing keccaks, so you should never rely on storing something there unless you know you're going to retrieve it immediately.

Actually it looks like the scratch space could be used by solidity even in-between inline assembly statements, so it's not safe to ever use (except for throwaway values). Will use the free memory pointer.

izqui · 2019-01-03T09:42:20Z

contracts/test/mocks/TokenReturnMissingMock.sol

+import "../../lib/math/SafeMath.sol";
+
+
+contract TokenReturnMissingMock {


Maybe inherit from the GeneralERC20 interface?

Don't think this is necessary, especially as the other token mocks wouldn't inherit from it.

izqui · 2019-01-03T09:49:11Z

contracts/common/SafeERC20.sol

+    */
+    function safeTransfer(ERC20 _token, address _to, uint256 _amount) internal returns (bool) {
+        GeneralERC20(_token).transfer(_to, _amount);
+        return checkSuccess();


I'm actually wondering whether all these 'safe' functions should provide a unified way to handle errors.

Right now depending on implementation details at the token level safeTransfer can either revert or return false to communicate that the transfer has failed. I think it would be a much better API if the call was really safe and it always returned false on transfer errors (by doing a raw rather than using the standard Solidity function calling syntax that just reverts on reverts).

sohkai · 2019-02-08T14:19:33Z

contracts/common/SafeERC20.sol

+                0x20                  // uint256 return
+            )
+
+            if gt(success, 0) {


It turns out this is a micro optimization over switch. In particular, the docs say call will only ever return 1 on success, but everyone seems to use case 0 for revert (and everything else as success) and so this mimics that behaviour.

sohkai · 2019-02-08T14:20:45Z

@bingen @izqui Would be great if you could review SafeERC20 again, given the assembly changes for #469 (comment).

Turns out using call in assembly results in much, much smaller (~100 bytes) bytecode than address.call.

bingen

Looking good! Really nice job!!!

bingen · 2019-02-08T21:16:39Z

test/recovery_to_vault.js

+    {
+      title: 'standards compliant, reverting token',
+      tokenContract: TokenMock,
+      revertsOnFailure: true,


Where are these being used?

Ah good catch, I must've copied those over from the safe_erc20 tests and not used them (since everything should revert in VaultDepositable)!

contracts/common/SafeERC20.sol

izqui · 2019-02-11T11:43:00Z

contracts/common/SafeERC20.sol

+
+    function invokeAndCheckSuccess(address _addr, bytes memory _calldata)
+        private
+        returns (bool ret)


I would not use an implicit name return value for this, and I would explicitly have bool ret = false in the function body and then have an explicit return ret as it makes the function easier to understand

Add SafeERC20

2f87df2

sohkai added the priority: high label Dec 17, 2018

sohkai requested review from izqui and bingen December 17, 2018 09:46

sohkai added 2 commits December 17, 2018 10:50

VaultRecoverable: use SafeERC20

02dcf6e

test: rename test file

ae1fcd5

sohkai force-pushed the safe-erc20 branch from 29feb6f to ae1fcd5 Compare December 17, 2018 09:50

sohkai commented Dec 17, 2018

View reviewed changes

cosmetic(SafeERC20): prefix arguments with _

9671921

bingen approved these changes Dec 17, 2018

View reviewed changes

sohkai removed the priority: high label Dec 17, 2018

sohkai mentioned this pull request Dec 17, 2018

Finance, Vault: Use SafeERC20 aragon/aragon-apps#614

Merged

sohkai added 2 commits December 17, 2018 21:26

cosmetic: fix indenting for SafeERC20Mock

af698ea

cosmetic(VaultRecoverable): avoid unnecessary cast

d7a21e9

sohkai mentioned this pull request Dec 17, 2018

feat(SafeERC20): add static call utilities #470

Merged

izqui reviewed Dec 17, 2018

View reviewed changes

sohkai added this to the A1 Sprint: 3.1 milestone Jan 2, 2019

izqui approved these changes Jan 3, 2019

View reviewed changes

izqui reviewed Jan 3, 2019

View reviewed changes

izqui mentioned this pull request Jan 4, 2019

Staking aragon/staking#1

Merged

sohkai modified the milestones: A1 Sprint: 3.1, A1 Sprint: 3.2 Jan 14, 2019

sohkai modified the milestones: A1 Sprint: 3.2, A1 Sprint: 4.1 Feb 4, 2019

sohkai added 6 commits February 8, 2019 12:56

Merge branch 'dev' into safe-erc20

9bba658

feat(SafeERC20): add static call utilities (#470)

ecaff5a

SafeERC20: add natspec to static call functions

71a705f

SafeERC20: never revert

2fc061a

SafeERC20: optimize call

31885fd

SafeERC20: use free memory pointer instead of scratch space

e5c40e7

sohkai commented Feb 8, 2019

View reviewed changes

SafeERC20: fix solidity-coverage

e6684af

bingen approved these changes Feb 8, 2019

View reviewed changes

test: small variable cleanup

179ca12

izqui approved these changes Feb 11, 2019

View reviewed changes

sohkai added 2 commits February 11, 2019 13:06

SafeERC20: remove unnecessary ERC20 casting

e4c075b

SafeERC20: use explicit returns

b26941b

sohkai merged commit a026e22 into dev Feb 11, 2019

sohkai deleted the safe-erc20 branch February 11, 2019 23:08

sohkai added a commit that referenced this pull request Mar 8, 2019

feat: Add SafeERC20 (#469)

e94109f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add SafeERC20 #469

feat: Add SafeERC20 #469

sohkai commented Dec 17, 2018 •

edited

Loading

sohkai Dec 17, 2018

bingen Dec 17, 2018

coveralls commented Dec 17, 2018 •

edited

Loading

bingen left a comment

bingen Dec 17, 2018

izqui Dec 17, 2018 •

edited

Loading

izqui Dec 17, 2018

izqui commented Dec 17, 2018

izqui left a comment

izqui Jan 3, 2019

sohkai Jan 11, 2019

sohkai Feb 8, 2019

izqui Jan 3, 2019

sohkai Feb 8, 2019

izqui Jan 3, 2019

sohkai Feb 8, 2019

sohkai commented Feb 8, 2019 •

edited

Loading

bingen left a comment

bingen Feb 8, 2019

sohkai Feb 8, 2019

izqui Feb 11, 2019

		import "../../lib/math/SafeMath.sol";


		contract TokenReturnMissingMock {

feat: Add SafeERC20 #469

feat: Add SafeERC20 #469

Conversation

sohkai commented Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 17, 2018 • edited Loading

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izqui Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izqui commented Dec 17, 2018

izqui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Feb 8, 2019 • edited Loading

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Dec 17, 2018 •

edited

Loading

coveralls commented Dec 17, 2018 •

edited

Loading

izqui Dec 17, 2018 •

edited

Loading

sohkai commented Feb 8, 2019 •

edited

Loading