-
Notifications
You must be signed in to change notification settings - Fork 465
Exchange Proxy: Address audit feedback (2) #2597
Exchange Proxy: Address audit feedback (2) #2597
Conversation
@@ -98,7 +98,7 @@ | |||
}, | |||
{ | |||
"note": "Add `setOperators()` to `IDydx`", | |||
"pr": "TODO" | |||
"pr": 2462 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retroactively fixing this changelog.
/// @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. |
There was a problem hiding this comment.
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 😢
d61a9ec
to
a6ea7c0
Compare
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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.
f0462b8
to
7b3e7c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Description
Aside from documentation stuff:
TransformERC20 feature
IERC20Transformer.transform()
goes back to returningTRANSFORMER_SUCCESS
.Transformation
struct, we pass auint32 deploymentNonce
and derive the transformer address before calling it.AllowanceTarget.executeCall
notpayable
._transformERC20Private()
private
.onlyOwner
setTransformerDeployer()
function to upgrade the deployer.Testing instructions
Types of changes
Checklist:
[WIP]
if necessary.