Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Exchange Proxy: Address audit feedback (2) #2597

Merged
merged 6 commits into from
Jun 9, 2020

Conversation

dorothy-zbornak
Copy link
Contributor

@dorothy-zbornak dorothy-zbornak commented Jun 4, 2020

Description

Aside from documentation stuff:

TransformERC20 feature

  • IERC20Transformer.transform() goes back to returning TRANSFORMER_SUCCESS.
  • Instead of passing a transformer address into a Transformation struct, we pass a uint32 deploymentNonce and derive the transformer address before calling it.
  • Make AllowanceTarget.executeCall not payable.
  • Make _transformERC20Private() private.
  • Add an onlyOwner setTransformerDeployer() function to upgrade the deployer.

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@@ -98,7 +98,7 @@
},
{
"note": "Add `setOperators()` to `IDydx`",
"pr": "TODO"
"pr": 2462
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Retroactively fixing this changelog.

Comment on lines 38 to 44
/// @dev Whether an address is authorized to call privileged functions.
/// @param 0 Address to query.
/// @return 0 Whether the address is authorized.
// @dev Whether an address is authorized to call privileged functions.
// @param 0 Address to query.
// @return 0 Whether the address is authorized.
mapping (address => bool) public override authorized;
/// @dev Whether an address is authorized to call privileged functions.
/// @param 0 Index of authorized address.
/// @return 0 Authorized address.
// @dev Whether an address is authorized to call privileged functions.
// @param 0 Index of authorized address.
// @return 0 Authorized address.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

solidity 0.6.9 now freaks out if you try to devdoc public storage vars 😢

@dorothy-zbornak dorothy-zbornak force-pushed the fix/zero-ex/transform-erc20-audit branch from d61a9ec to a6ea7c0 Compare June 4, 2020 20:28
@dorothy-zbornak dorothy-zbornak marked this pull request as ready for review June 4, 2020 20:28
@dorothy-zbornak dorothy-zbornak changed the title Exchange Proxy: Address audit feedback Exchange Proxy: Address audit feedback (2) Jun 4, 2020
Copy link
Contributor

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

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

qq about the new deployment behavior

}

/// @dev Initialize and register this feature.
/// Should be delegatecalled by `Migrate.migrate()`.
function migrate() external returns (bytes4 success) {
/// @param transformerDeployer The trusted deployer for transformers.
function migrate(address transformerDeployer) external returns (bytes4 success) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@return

return LibMigrate.MIGRATE_SUCCESS;
}

/// @dev Replace the allowed deployer for transformers.
/// Only callable by the owner.
function setTransformerDeployer(address transformerDeployer)
Copy link
Contributor

Choose a reason for hiding this comment

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

@param

address payable taker,
bytes32 callDataHash
)
private
{
// Derive the transformer address from the deployment nonce.
Copy link
Contributor

Choose a reason for hiding this comment

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

so this means that if we ever replace the transformerDeployer address, all transformers deployed by the old deployer become inaccessible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. We'll have to redeploy the existing ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

what was the rationale for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code here or the deployer being replaceable?

Copy link
Contributor

Choose a reason for hiding this comment

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

well the deployer being replaceable and the fact that old deployments become defunct once a deployer is replaced

Copy link
Contributor Author

@dorothy-zbornak dorothy-zbornak Jun 5, 2020

Choose a reason for hiding this comment

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

I decided to make it replaceable based off internal discussions and the uncertainty over how we want to manage the deployer at this stage. Previously we would have had to redeploy this entire feature to change the deployer. The fact that the transformers would die with the deployer is not new, because we always used the nonce to assert validity. This might actually be a good thing since it's a universal off switch for transformers if the deployer account is compromised. Ideally we won't be changing the deployer often, and we'll probably only have a handful of transformers early on.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 79.487% when pulling f0462b8 on fix/zero-ex/transform-erc20-audit into 32793cc on development.

Copy link
Contributor

@hysz hysz left a comment

Choose a reason for hiding this comment

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

LGTM! A couple general comments.

@@ -220,31 +241,44 @@ contract TransformERC20 is
.getSpendableERC20BalanceOf(inputToken, taker);
}

IFlashWallet wallet = getTransformWallet();
TransformERC20PrivateState memory state;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this to work around a stack too deep error? The general downside to this approach is that we're now declaring w/o initializing, so a developer may accidentally reference a field before it's been populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I don't think there's a way around that problem.

address payable transformer = LibERC20Transformer.getDeployedAddress(
transformerDeployer,
transformation.deploymentNonce
);
// Call `transformer.transform()` as the wallet.
bytes memory resultData = wallet.executeDelegateCall(
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional. If this will be reused then could be good to encapsulate in a library, like LibFlashWallet, that both executes the call and validates the return value.

@dorothy-zbornak dorothy-zbornak force-pushed the fix/zero-ex/transform-erc20-audit branch from f0462b8 to 7b3e7c9 Compare June 8, 2020 19:28
Copy link
Contributor

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

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

lgtm!

@dorothy-zbornak dorothy-zbornak merged commit 967eff4 into development Jun 9, 2020
@dorothy-zbornak dorothy-zbornak deleted the fix/zero-ex/transform-erc20-audit branch June 9, 2020 00:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants