Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enhance approveAndCall logic #84

Merged
merged 16 commits into from
Apr 8, 2020
Merged

Enhance approveAndCall logic #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
173d21d
Add flash approveAndCall logic
elenadimitrova Apr 1, 2020
be80a3f
Add test for new flash approveAndCall logic
elenadimitrova Apr 1, 2020
9114463
Refactor approveAndCall logic into the BaseTransfer module
elenadimitrova Apr 3, 2020
895d2f7
Make SafeMath available to all modules
elenadimitrova Apr 3, 2020
cff6634
Update TransferManager to do BaseTransfer approveAndCall
elenadimitrova Apr 3, 2020
0ea986d
Enable different spender to caller in TransferManager approveAndCall
elenadimitrova Apr 3, 2020
8daf509
Fix rebase issue
elenadimitrova Apr 6, 2020
dc89490
Add TransferManager to list of modules to upgrade
elenadimitrova Apr 6, 2020
b39b60c
Replace LegacyTokenTransfer usage with instance of the 1.5 TransferMa…
elenadimitrova Apr 6, 2020
970c6bd
Update ApprovedAndCalledContract event definition to index spender
elenadimitrova Apr 6, 2020
d9a7706
Incude originally approved amount in the approveAndCall approval to s…
elenadimitrova Apr 6, 2020
4e2296b
Improve TransferManager code comments
elenadimitrova Apr 7, 2020
bb14864
Adjust test coverage thresholds for reshuffled LegacyTransferMaanger …
elenadimitrova Apr 7, 2020
8523af9
Check the approveAndCall hasn't spent more than the given amount
elenadimitrova Apr 7, 2020
57f4fc1
Conditionally update allowance based on whether it's changed
elenadimitrova Apr 8, 2020
9f2d735
Remove config.modules.TokenTransfer and use current TransferManager a…
elenadimitrova Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
501 changes: 501 additions & 0 deletions contracts/legacy/LegacyTransferManager.sol
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 1 addition & 3 deletions contracts/modules/ApprovedTransfer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import "../wallet/BaseWallet.sol";
import "./common/BaseModule.sol";
import "./common/RelayerModuleV2.sol";
import "./common/BaseTransfer.sol";
import "../../lib/utils/SafeMath.sol";

/**
* @title ApprovedTransfer
Expand Down Expand Up @@ -100,8 +99,7 @@ contract ApprovedTransfer is BaseModule, RelayerModuleV2, BaseTransfer {
onlyWhenUnlocked(_wallet)
{
require(!_wallet.authorised(_contract) && _contract != address(_wallet), "AT: Forbidden contract");
doApproveToken(_wallet, _token, _spender, _amount);
doCallContract(_wallet, _contract, 0, _data);
doApproveTokenAndCallContract(_wallet, _token, _spender, _amount, _contract, _data);
}

// *************** Implementation of RelayerModule methods ********************* //
Expand Down
1 change: 0 additions & 1 deletion contracts/modules/CompoundManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

pragma solidity ^0.5.4;

import "../../lib/utils/SafeMath.sol";
import "../wallet/BaseWallet.sol";
import "./common/BaseModule.sol";
import "./common/RelayerModule.sol";
Expand Down
1 change: 0 additions & 1 deletion contracts/modules/RecoveryManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import "../wallet/BaseWallet.sol";
import "./common/BaseModule.sol";
import "./common/RelayerModuleV2.sol";
import "./storage/GuardianStorage.sol";
import "../../lib/utils/SafeMath.sol";

/**
* @title RecoveryManager
Expand Down
1 change: 0 additions & 1 deletion contracts/modules/TokenExchanger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import "../wallet/BaseWallet.sol";
import "./common/BaseModule.sol";
import "./common/RelayerModule.sol";
import "./common/OnlyOwnerModule.sol";
import "../../lib/utils/SafeMath.sol";
import "../../lib/other/ERC20.sol";
import "../../lib/other/KyberNetwork.sol";

Expand Down
31 changes: 11 additions & 20 deletions contracts/modules/TransferManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,17 @@ contract TransferManager is BaseModule, RelayerModule, OnlyOwnerModule, BaseTran
* We assume that the contract will pull the tokens and does not require ETH.
* @param _wallet The target wallet.
* @param _token The token to approve.
* @param _contract The address of the contract.
* @param _spender The address to approve.
* @param _amount The amount of ERC20 tokens to approve.
* @param _contract The address of the contract.
* @param _data The encoded method data
*/
function approveTokenAndCallContract(
BaseWallet _wallet,
address _token,
address _contract,
address _spender,
uint256 _amount,
address _contract,
bytes calldata _data
)
external
Expand All @@ -258,25 +260,14 @@ contract TransferManager is BaseModule, RelayerModule, OnlyOwnerModule, BaseTran
// Make sure we don't call a module, the wallet itself, or a supported ERC20
authoriseContractCall(_wallet, _contract);

if (isWhitelisted(_wallet, _contract)) {
doApproveToken(_wallet, _token, _contract, _amount);
doCallContract(_wallet, _contract, 0, _data);
} else {
// get current alowance
uint256 currentAllowance = ERC20(_token).allowance(address(_wallet), _contract);
if (_amount <= currentAllowance) {
// no need to approve more
doCallContract(_wallet, _contract, 0, _data);
} else {
// check if delta is under the limit
uint delta = _amount - currentAllowance;
uint256 deltaInEth = priceProvider.getEtherValue(delta, _token);
require(checkAndUpdateDailySpent(_wallet, deltaInEth), "TM: Approve above daily limit");
// approve if under the limit
doApproveToken(_wallet, _token, _contract, _amount);
doCallContract(_wallet, _contract, 0, _data);
}
if (!isWhitelisted(_wallet, _contract)) {
// check if the amount is under the daily limit
// check the entire amount because the currently approved amount will be restored and should still count towards the daily limit
uint256 valueInEth = priceProvider.getEtherValue(_amount, _token);
require(checkAndUpdateDailySpent(_wallet, valueInEth), "TM: Approve above daily limit");
}

doApproveTokenAndCallContract(_wallet, _token, _spender, _amount, _contract, _data);
}

/**
Expand Down
1 change: 1 addition & 0 deletions contracts/modules/common/BaseModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import "../../infrastructure/ModuleRegistry.sol";
import "../storage/GuardianStorage.sol";
import "./Module.sol";
import "../../../lib/other/ERC20.sol";
import "../../../lib/utils/SafeMath.sol";

/**
* @title BaseModule
Expand Down
61 changes: 60 additions & 1 deletion contracts/modules/common/BaseTransfer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,15 @@ contract BaseTransfer is BaseModule {
event Transfer(address indexed wallet, address indexed token, uint256 indexed amount, address to, bytes data);
event Approved(address indexed wallet, address indexed token, uint256 amount, address spender);
event CalledContract(address indexed wallet, address indexed to, uint256 amount, bytes data);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// *************** Internal Functions ********************* //

/**
Expand Down Expand Up @@ -77,4 +85,55 @@ contract BaseTransfer is BaseModule {
invokeWallet(address(_wallet), _contract, _value, _data);
emit CalledContract(address(_wallet), _contract, _value, _data);
}

/**
* @dev Helper method to approve a certain amount of token and call an external contract.
* The address that spends the _token and the address that is called with _data can be different.
* @param _wallet The target wallet.
* @param _token The ERC20 address.
* @param _spender The spender address.
* @param _amount The amount of tokens to transfer.
* @param _contract The contract address.
* @param _data The method data.
*/
function doApproveTokenAndCallContract(
BaseWallet _wallet,
address _token,
address _spender,
uint256 _amount,
address _contract,
bytes memory _data
)
internal
{
uint256 existingAllowance = ERC20(_token).allowance(address(_wallet), _spender);
uint256 totalAllowance = SafeMath.add(existingAllowance, _amount);
// Approve the desired amount plus existing amount. This logic allows for potential gas saving later
// when restoring the original approved amount, in cases where the _spender uses the exact approved _amount.
bytes memory methodData = abi.encodeWithSignature("approve(address,uint256)", _spender, totalAllowance);

invokeWallet(address(_wallet), _token, 0, methodData);
invokeWallet(address(_wallet), _contract, 0, _data);

// Calculate the approved amount that was spent after the call
uint256 unusedAllowance = ERC20(_token).allowance(address(_wallet), _spender);
uint256 usedAllowance = SafeMath.sub(totalAllowance, unusedAllowance);
// Ensure the amount spent does not exceed the amount approved for this call
require(usedAllowance <= _amount, "BT: insufficient amount for call");

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

emit ApprovedAndCalledContract(
address(_wallet),
_contract,
_spender,
_token,
_amount,
usedAllowance,
_data);
}
}
1 change: 0 additions & 1 deletion contracts/modules/common/LimitManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

pragma solidity ^0.5.4;
import "../../wallet/BaseWallet.sol";
import "../../../lib/utils/SafeMath.sol";
import "./BaseModule.sol";

/**
Expand Down
1 change: 0 additions & 1 deletion contracts/modules/maker/MakerManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

pragma solidity ^0.5.4;

import "../../../lib/utils/SafeMath.sol";
import "../../wallet/BaseWallet.sol";
import "../common/BaseModule.sol";
import "../common/RelayerModule.sol";
Expand Down
Loading