Enhance approveAndCall logic #84

elenadimitrova · 2020-04-01T10:58:22Z

In the current implementation of approveTokenAndCallContract, the amount of tokens approved overwrites any existing amount of the same token and spender. Here we add logic to restore the original approved amount after the approveTokenAndCallContract call in both (ApprovedTransfer and TransferManager) and emit a new dedicated event with the following definition:

event ApprovedAndCalledContract(
        address indexed wallet,
        address indexed to,
        address spender,
        address indexed token,
        uint256 amountApproved,
        uint256 amountSpent,
        bytes data
    );

This PR also enables a different contract address for the approved spender and the called contract for TransferManager module, analogous to behaviour already implemented for ApprovedTransfer in #73

Also I needed SafeMath in the BaseTransfer contract so pushing it there meant this was provisioned to both ApprovedTransfer and TransferManager.

We also decommission the config.modules.TokenTransfer config entry.

juniset

We should add the deployment of the new TransferManager in 7_upgrade_1_6.js.

juniset · 2020-04-06T07:50:02Z

contracts/modules/common/BaseTransfer.sol

+        uint256 currentAllowance = ERC20(_token).allowance(address(_wallet), _spender);
+
+        // Approve the desired amount
+        bytes memory methodData = abi.encodeWithSignature("approve(address,uint256)", _spender, _amount);


I think we should here approve currentAllowance + _amount so if unusedAllowance == currentAllowance after the contract call (which should be the majority of the cases) then we don't need to make the call on L121 and we save gas.

The problem with that is that the new allowance amount left can be different to the original allowance.

Also we would be allowing that TX to use your existing allowance, so should set explicit amount before the call, make the call, and then reset to old allowance

Should we error if the called contract consumes more than they approved in this call (spending additionally from their original approved amount)?

juniset · 2020-04-06T07:56:21Z

contracts/modules/common/BaseTransfer.sol

+        uint256 amountApproved,
+        uint256 amountSpent,
+        bytes data
+    );


Maybe we should index wallet token and amountSpent to mirror what we index in Transfer ? I suspect that it is the main information the clients are interested in. Maybe check with @th3m477 ?

I don't think uint256 values need to be indexed generally, makes no client difference here as it's a new event.

Addresses make more sense as we are more likely to do exploratory network analysis in the future, or quick filters (i.e. "show me all the transfers I made to X address"), so more of a case for spender, but up to you @juniset @elenadimitrova

Actually we can only have up to 3 indexed arguments per event so leaving as is.

juniset · 2020-04-07T12:52:04Z

contracts/modules/common/BaseTransfer.sol

+
+        // Restore the original allowance amount.
+        methodData = abi.encodeWithSignature("approve(address,uint256)", _spender, existingAllowance);
+        invokeWallet(address(_wallet), _token, 0, methodData);


I think there is an IF missing if we want to save some gas and it should be:

if (unusedAllowance != existingAllowance) { // Restore the original allowance amount. methodData = abi.encodeWithSignature("approve(address,uint256)", _spender, existingAllowance); invokeWallet(address(_wallet), _token, 0, methodData); }

I intentionally omitted it as we won't pay for storage updates when the value doesn't change. So implicitly this is enforced anyway while keeping the code simpler.

Yes, but I believe there will still be quite some gas consumed by invoking the wallet: internal call to the wallet, then delegatecall to the wallet implementation, then checking that the calling address is an authorised module, then calling the token contract which itself need to do a few check before updating the memory slot (even if this last step is free).

It's around 12470 gas which is approx 1 cent worth at current prices. I can't tell whether the complexity outweighs the advantage but I'll add it.

juniset · 2020-04-07T13:07:00Z

deployment/7_upgrade_1_6.js

+    config.settings.securityPeriod || 0,
+    config.settings.securityWindow || 0,
+    config.settings.defaultLimit || "1000000000000000000",
+    ["test", "staging", "prod"].includes(network) ? config.modules.TokenTransfer : "0x0000000000000000000000000000000000000000",


It should be config.modules.TransferManager and not config.modules.TokenTransfer.

But config.modules.TokenTransfer is the address of the old config.modules.TransferManager no?

Maybe when upgrading the TransferManager we should copy its old value here though

It is the address of the first version of the TransferManager, yes. But here we are going from the second version (config.modules.TransferManager) to the third one. So the new TransferManager needs to read the state of a wallet's limit from the second version which is config.modules.TransferManager.

But we update the config.modules.TransferManager address to the new one in the same script.

Okay, removed TokenTransfer in favour of using TransferManager value prior to upgrade updates.

juniset · 2020-04-07T13:14:54Z

deployment/7_upgrade_1_6.js


 const utils = require("../utils/utilities.js");

 const TARGET_VERSION = "1.6.0";
-const MODULES_TO_ENABLE = ["ApprovedTransfer", "RecoveryManager", "MakerV2Manager"];
+const MODULES_TO_ENABLE = ["ApprovedTransfer", "RecoveryManager", "MakerV2Manager", "TransferManager"];
 const MODULES_TO_DISABLE = ["UniswapManager"];

 const BACKWARD_COMPATIBILITY = 3;


Didn't we say we would set BACKWARD_COMPATIBILITY to 1 and remove the part of the script where we create a LegacyUpgrader? Or are we addressing this in a separate PR?

There's a separate PR for it #91 as I wanted to double check the clients can support the upgrade through 1.4 -> 1.6

Which restores the original approved amount after an approveAndCall and emits a new dedicated event

Rename contract instances to match the contract types they represent

…nager

…ave gas

…contract

…ddress prior to upgrades, otherwise use 0x0

elenadimitrova self-assigned this Apr 1, 2020

elenadimitrova marked this pull request as ready for review April 1, 2020 14:13

elenadimitrova requested a review from juniset April 2, 2020 06:19

elenadimitrova changed the base branch from develop to release/1.6.0 April 2, 2020 07:26

elenadimitrova changed the base branch from release/1.6.0 to develop April 2, 2020 07:31

elenadimitrova force-pushed the feature/flash-approve-and-call branch 3 times, most recently from 3d51aab to 2416398 Compare April 3, 2020 10:44

elenadimitrova removed the request for review from juniset April 3, 2020 13:55

elenadimitrova changed the title ~~Add flash approveAndCall logic~~ Enhance approveAndCall logic Apr 6, 2020

juniset reviewed Apr 6, 2020

View reviewed changes

elenadimitrova force-pushed the feature/flash-approve-and-call branch 3 times, most recently from e002379 to cd877b1 Compare April 6, 2020 20:45

elenadimitrova requested a review from juniset April 7, 2020 07:45

juniset reviewed Apr 7, 2020

View reviewed changes

elenadimitrova added 14 commits April 8, 2020 10:24

Add flash approveAndCall logic

173d21d

Which restores the original approved amount after an approveAndCall and emits a new dedicated event

Add test for new flash approveAndCall logic

be80a3f

Rename contract instances to match the contract types they represent

Refactor approveAndCall logic into the BaseTransfer module

9114463

Make SafeMath available to all modules

895d2f7

Update TransferManager to do BaseTransfer approveAndCall

cff6634

Enable different spender to caller in TransferManager approveAndCall

0ea986d

Fix rebase issue

8daf509

Add TransferManager to list of modules to upgrade

dc89490

Replace LegacyTokenTransfer usage with instance of the 1.5 TransferMa…

b39b60c

…nager

Update ApprovedAndCalledContract event definition to index spender

970c6bd

Incude originally approved amount in the approveAndCall approval to s…

d9a7706

…ave gas

Improve TransferManager code comments

4e2296b

Adjust test coverage thresholds for reshuffled LegacyTransferMaanger …

bb14864

…contract

Check the approveAndCall hasn't spent more than the given amount

8523af9

elenadimitrova force-pushed the feature/flash-approve-and-call branch from 1bfa7b1 to 8523af9 Compare April 8, 2020 07:24

elenadimitrova added 2 commits April 8, 2020 13:57

Conditionally update allowance based on whether it's changed

57f4fc1

Remove config.modules.TokenTransfer and use current TransferManager a…

9f2d735

…ddress prior to upgrades, otherwise use 0x0

juniset approved these changes Apr 8, 2020

View reviewed changes

juniset merged commit 7a70918 into develop Apr 8, 2020

elenadimitrova deleted the feature/flash-approve-and-call branch April 8, 2020 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance approveAndCall logic #84

Enhance approveAndCall logic #84

elenadimitrova commented Apr 1, 2020 •

edited

Loading

juniset left a comment

juniset Apr 6, 2020

elenadimitrova Apr 6, 2020

th3m477 Apr 6, 2020

elenadimitrova Apr 6, 2020 •

edited

Loading

juniset Apr 6, 2020

th3m477 Apr 6, 2020

elenadimitrova Apr 6, 2020

juniset Apr 7, 2020

elenadimitrova Apr 8, 2020

juniset Apr 8, 2020

elenadimitrova Apr 8, 2020

juniset Apr 7, 2020

elenadimitrova Apr 8, 2020

elenadimitrova Apr 8, 2020

juniset Apr 8, 2020

elenadimitrova Apr 8, 2020

elenadimitrova Apr 8, 2020

juniset Apr 7, 2020

elenadimitrova Apr 8, 2020

Enhance approveAndCall logic #84

Enhance approveAndCall logic #84

Conversation

elenadimitrova commented Apr 1, 2020 • edited Loading

juniset 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

elenadimitrova Apr 6, 2020 • edited Loading

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

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

elenadimitrova commented Apr 1, 2020 •

edited

Loading

elenadimitrova Apr 6, 2020 •

edited

Loading