feature(AuthMultisig): removing auth logic from the gateway #110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # contracts/AxelarGateway.sol # contracts/AxelarGatewayMultisig.sol # contracts/AxelarGatewaySinglesig.sol # contracts/interfaces/IAxelarGateway.sol
fish-sammy
requested changes
Jun 11, 2022
contracts/AxelarAuthMultisig.sol
Outdated
Comment on lines
102
to
125
| uint256 operatorsCount = operators.length; | ||
| uint256 operatorsIndex = 0; | ||
| uint256 signatureCount = signatures.length; | ||
|
|
||
| // looking for signers within operators | ||
| // assuming that both operators and signers are sorted | ||
| for (uint256 i; i < signatureCount; ++i) { | ||
| address signer = ECDSA.recover(messageHash, signatures[i]); | ||
| if (signer == operators[operatorsIndex]) { | ||
| ++operatorsIndex; | ||
| } else { | ||
| // keep looping through operators to find the signer | ||
| while (operatorsIndex < operatorsCount) { | ||
| ++operatorsIndex; | ||
| if (signer == operators[operatorsIndex]) break; | ||
| } | ||
| // check if we ran out of operators | ||
| if (operatorsIndex < operatorsCount) { | ||
| ++operatorsIndex; | ||
| } else { | ||
| revert MalformedSigners(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggested change
| uint256 operatorsCount = operators.length; | |
| uint256 operatorsIndex = 0; | |
| uint256 signatureCount = signatures.length; | |
| // looking for signers within operators | |
| // assuming that both operators and signers are sorted | |
| for (uint256 i; i < signatureCount; ++i) { | |
| address signer = ECDSA.recover(messageHash, signatures[i]); | |
| if (signer == operators[operatorsIndex]) { | |
| ++operatorsIndex; | |
| } else { | |
| // keep looping through operators to find the signer | |
| while (operatorsIndex < operatorsCount) { | |
| ++operatorsIndex; | |
| if (signer == operators[operatorsIndex]) break; | |
| } | |
| // check if we ran out of operators | |
| if (operatorsIndex < operatorsCount) { | |
| ++operatorsIndex; | |
| } else { | |
| revert MalformedSigners(); | |
| } | |
| } | |
| } | |
| uint256 j = 0; | |
| // looking for signers within operators | |
| // assuming that both operators and signatures are sorted | |
| for (uint256 i; i < signatures.length; ++i) { | |
| address signer = ECDSA.recover(messageHash, signatures[i]); | |
| for (; j < operators.length && signer != operators[j]; ++j) {} | |
| if (j == operators.length) revert MalformedSigners(); | |
| } |
There was a problem hiding this comment.
Nice simplification! It became this
uint256 j = 0;
// looking for signers within operators
// assuming that both operators and signatures are sorted
for (uint256 i = 0; i < signatures.length; ++i) {
address signer = ECDSA.recover(messageHash, signatures[i]);
// looping through remaining operators to find a match
for (; j < operators.length && signer != operators[j]; ++j) {}
if (j == operators.length) revert MalformedSigners();
// increasing operators index if match was found
++j;
}
contracts/AxelarGateway.sol
Outdated
| tokenAddress, | ||
| abi.encodeWithSelector(IERC20.transferFrom.selector, sender, address(this), amount) | ||
| ); | ||
| if (operatorshipParams.length > 0) { |
There was a problem hiding this comment.
When should operatorshipParams be empty?
There was a problem hiding this comment.
If we don't want to transfer operatorship on upgrade?
There was a problem hiding this comment.
On initial deployment we want to pass admins, but not the operators, because gateway is not the owner of the auth yet
fish-sammy
reviewed
Jun 16, 2022
contracts/AxelarAuthMultisig.sol
Outdated
|
|
||
| uint8 internal constant OLD_KEY_RETENTION = 16; | ||
|
|
||
| constructor(bytes memory recentOperators) { |
There was a problem hiding this comment.
Suggested change
| constructor(bytes memory recentOperators) { | |
| constructor(bytes[] memory recentOperators) { |
why not just?
contracts/AxelarGateway.sol
Outdated
Comment on lines
457
to
458
| uint256 signersRole; | ||
| (chainId, signersRole, commandIds, commands, params) = abi.decode(executeData, (uint256, uint256, bytes32[], string[], bytes[])); |
There was a problem hiding this comment.
Suggested change
| uint256 signersRole; | |
| (chainId, signersRole, commandIds, commands, params) = abi.decode(executeData, (uint256, uint256, bytes32[], string[], bytes[])); | |
| (chainId, , commandIds, commands, params) = abi.decode(executeData, (uint256, uint256, bytes32[], string[], bytes[])); |
There was a problem hiding this comment.
Applied, my IDE is complaining, but it compiles with no problems
contracts/AxelarGateway.sol
Outdated
| bytes[] memory params_ | ||
| ) { | ||
| (chainId, commandIds, commands, params) = (chainId_, commandIds_, commands_, params_); | ||
| } catch (bytes memory) { |
There was a problem hiding this comment.
Suggested change
| } catch (bytes memory) { | |
| } catch { |
fish-sammy
approved these changes
Jun 16, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
AxelarGatewayMultisigandAxelarGatewaySinglesigimplementationsAxelarGatewayIAxelarAuthModuleinterfaceAxelarAuthMultisigmodule implementing the interface