From 6fc38292f2a63a3a7dfade871cba5841781e9d68 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Thu, 1 Aug 2019 13:59:19 -0700 Subject: [PATCH] Add RichErrors for Authorizable --- contracts/utils/compiler.json | 1 + .../utils/contracts/src/Authorizable.sol | 62 ++++----- .../src/LibAuthorizableRichErrors.sol | 119 ++++++++++++++++++ contracts/utils/package.json | 2 +- contracts/utils/src/artifacts.ts | 2 + contracts/utils/src/wrappers.ts | 1 + contracts/utils/test/authorizable.ts | 72 +++++------ contracts/utils/tsconfig.json | 1 + packages/types/src/index.ts | 1 - .../utils/src/authorizable_revert_errors.ts | 69 ++++++++++ packages/utils/src/index.ts | 2 + 11 files changed, 257 insertions(+), 75 deletions(-) create mode 100644 contracts/utils/contracts/src/LibAuthorizableRichErrors.sol create mode 100644 packages/utils/src/authorizable_revert_errors.ts diff --git a/contracts/utils/compiler.json b/contracts/utils/compiler.json index 4ebfdb30ee..ebd1f5898c 100644 --- a/contracts/utils/compiler.json +++ b/contracts/utils/compiler.json @@ -32,6 +32,7 @@ "src/Ownable.sol", "src/ReentrancyGuard.sol", "src/SafeMath.sol", + "src/interfaces/IAuthorizable.sol", "src/interfaces/IOwnable.sol", "test/TestConstants.sol", "test/TestLibAddress.sol", diff --git a/contracts/utils/contracts/src/Authorizable.sol b/contracts/utils/contracts/src/Authorizable.sol index a546f2be1e..cbd8ac475c 100644 --- a/contracts/utils/contracts/src/Authorizable.sol +++ b/contracts/utils/contracts/src/Authorizable.sol @@ -18,8 +18,10 @@ pragma solidity ^0.5.9; -import "./Ownable.sol"; import "./interfaces/IAuthorizable.sol"; +import "./LibAuthorizableRichErrors.sol"; +import "./LibRichErrors.sol"; +import "./Ownable.sol"; contract Authorizable is @@ -28,10 +30,9 @@ contract Authorizable is { /// @dev Only authorized addresses can invoke functions with this modifier. modifier onlyAuthorized { - require( - authorized[msg.sender], - "SENDER_NOT_AUTHORIZED" - ); + if (!authorized[msg.sender]) { + LibRichErrors._rrevert(LibAuthorizableRichErrors.SenderNotAuthorizedError(msg.sender)); + } _; } @@ -44,14 +45,15 @@ contract Authorizable is external onlyOwner { - require( - target != address(0), - "ZERO_CANT_BE_AUTHORIZED" - ); - require( - !authorized[target], - "TARGET_ALREADY_AUTHORIZED" - ); + // Ensure that the target is not the zero address. + if (target == address(0)) { + LibRichErrors._rrevert(LibAuthorizableRichErrors.ZeroCantBeAuthorizedError()); + } + + // Ensure that the target is not already authorized. + if (authorized[target]) { + LibRichErrors._rrevert(LibAuthorizableRichErrors.TargetAlreadyAuthorizedError(target)); + } authorized[target] = true; authorities.push(target); @@ -64,10 +66,9 @@ contract Authorizable is external onlyOwner { - require( - authorized[target], - "TARGET_NOT_AUTHORIZED" - ); + if (!authorized[target]) { + LibRichErrors._rrevert(LibAuthorizableRichErrors.TargetNotAuthorizedError(target)); + } delete authorized[target]; for (uint256 i = 0; i < authorities.length; i++) { @@ -90,18 +91,21 @@ contract Authorizable is external onlyOwner { - require( - authorized[target], - "TARGET_NOT_AUTHORIZED" - ); - require( - index < authorities.length, - "INDEX_OUT_OF_BOUNDS" - ); - require( - authorities[index] == target, - "AUTHORIZED_ADDRESS_MISMATCH" - ); + if (!authorized[target]) { + LibRichErrors._rrevert(LibAuthorizableRichErrors.TargetNotAuthorizedError(target)); + } + if (index >= authorities.length) { + LibRichErrors._rrevert(LibAuthorizableRichErrors.IndexOutOfBoundsError( + index, + authorities.length + )); + } + if (authorities[index] != target) { + LibRichErrors._rrevert(LibAuthorizableRichErrors.AuthorizedAddressMismatchError( + authorities[index], + target + )); + } delete authorized[target]; authorities[index] = authorities[authorities.length - 1]; diff --git a/contracts/utils/contracts/src/LibAuthorizableRichErrors.sol b/contracts/utils/contracts/src/LibAuthorizableRichErrors.sol new file mode 100644 index 0000000000..5cec572a4e --- /dev/null +++ b/contracts/utils/contracts/src/LibAuthorizableRichErrors.sol @@ -0,0 +1,119 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; + + +library LibAuthorizableRichErrors { + + // bytes4(keccak256("AuthorizedAddressMismatchError(address,address)")) + bytes4 internal constant AUTHORIZED_ADDRESS_MISMATCH_ERROR_SELECTOR = + 0x140a84db; + + // bytes4(keccak256("IndexOutOfBoundsError(uint256,uint256)")) + bytes4 internal constant INDEX_OUT_OF_BOUNDS_ERROR_SELECTOR = + 0xe9f83771; + + // bytes4(keccak256("SenderNotAuthorizedError(address)")) + bytes4 internal constant SENDER_NOT_AUTHORIZED_ERROR_SELECTOR = + 0xb65a25b9; + + // bytes4(keccak256("TargetAlreadyAuthorizedError(address)")) + bytes4 internal constant TARGET_ALREADY_AUTHORIZED_ERROR_SELECTOR = + 0xde16f1a0; + + // bytes4(keccak256("TargetNotAuthorizedError(address)")) + bytes4 internal constant TARGET_NOT_AUTHORIZED_ERROR_SELECTOR = + 0xeb5108a2; + + // bytes4(keccak256("ZeroCantBeAuthorizedError()")) + bytes internal constant ZERO_CANT_BE_AUTHORIZED_ERROR_BYTES = + hex"57654fe4"; + + // solhint-disable func-name-mixedcase + function AuthorizedAddressMismatchError( + address authorized, + address target + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + AUTHORIZED_ADDRESS_MISMATCH_ERROR_SELECTOR, + authorized, + target + ); + } + + function IndexOutOfBoundsError( + uint256 index, + uint256 length + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + INDEX_OUT_OF_BOUNDS_ERROR_SELECTOR, + index, + length + ); + } + + function SenderNotAuthorizedError(address sender) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + SENDER_NOT_AUTHORIZED_ERROR_SELECTOR, + sender + ); + } + + function TargetAlreadyAuthorizedError(address target) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + TARGET_ALREADY_AUTHORIZED_ERROR_SELECTOR, + target + ); + } + + function TargetNotAuthorizedError(address target) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + TARGET_NOT_AUTHORIZED_ERROR_SELECTOR, + target + ); + } + + function ZeroCantBeAuthorizedError() + internal + pure + returns (bytes memory) + { + return ZERO_CANT_BE_AUTHORIZED_ERROR_BYTES; + } +} diff --git a/contracts/utils/package.json b/contracts/utils/package.json index 60bed25039..a369e1cd41 100644 --- a/contracts/utils/package.json +++ b/contracts/utils/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(Authorizable|IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddress|TestLibAddressArray|TestLibBytes|TestLibEIP712|TestLibRichErrors|TestOwnable|TestReentrancyGuard|TestSafeMath).json", + "abis": "./generated-artifacts/@(Authorizable|IAuthorizable|IOwnable|LibAddress|LibBytes|LibEIP1271|LibEIP712|Ownable|ReentrancyGuard|SafeMath|TestConstants|TestLibAddress|TestLibAddressArray|TestLibBytes|TestLibEIP712|TestLibRichErrors|TestOwnable|TestReentrancyGuard|TestSafeMath).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/utils/src/artifacts.ts b/contracts/utils/src/artifacts.ts index 9ce4522bb5..c9ae81ca55 100644 --- a/contracts/utils/src/artifacts.ts +++ b/contracts/utils/src/artifacts.ts @@ -6,6 +6,7 @@ import { ContractArtifact } from 'ethereum-types'; import * as Authorizable from '../generated-artifacts/Authorizable.json'; +import * as IAuthorizable from '../generated-artifacts/IAuthorizable.json'; import * as IOwnable from '../generated-artifacts/IOwnable.json'; import * as LibAddress from '../generated-artifacts/LibAddress.json'; import * as LibBytes from '../generated-artifacts/LibBytes.json'; @@ -32,6 +33,7 @@ export const artifacts = { Ownable: Ownable as ContractArtifact, ReentrancyGuard: ReentrancyGuard as ContractArtifact, SafeMath: SafeMath as ContractArtifact, + IAuthorizable: IAuthorizable as ContractArtifact, IOwnable: IOwnable as ContractArtifact, TestConstants: TestConstants as ContractArtifact, TestLibAddress: TestLibAddress as ContractArtifact, diff --git a/contracts/utils/src/wrappers.ts b/contracts/utils/src/wrappers.ts index b004152ce3..daf082ed59 100644 --- a/contracts/utils/src/wrappers.ts +++ b/contracts/utils/src/wrappers.ts @@ -4,6 +4,7 @@ * ----------------------------------------------------------------------------- */ export * from '../generated-wrappers/authorizable'; +export * from '../generated-wrappers/i_authorizable'; export * from '../generated-wrappers/i_ownable'; export * from '../generated-wrappers/lib_address'; export * from '../generated-wrappers/lib_bytes'; diff --git a/contracts/utils/test/authorizable.ts b/contracts/utils/test/authorizable.ts index 8a5fadf6ce..84b7764fe4 100644 --- a/contracts/utils/test/authorizable.ts +++ b/contracts/utils/test/authorizable.ts @@ -1,14 +1,12 @@ import { chaiSetup, constants, - expectTransactionFailedAsync, provider, txDefaults, web3Wrapper, } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; -import { RevertReason } from '@0x/types'; -import { BigNumber, OwnableRevertErrors } from '@0x/utils'; +import { AuthorizableRevertErrors, BigNumber, OwnableRevertErrors } from '@0x/utils'; import * as chai from 'chai'; import * as _ from 'lodash'; @@ -51,7 +49,7 @@ describe('Authorizable', () => { }); describe('addAuthorizedAddress', () => { - it('should throw if not called by owner', async () => { + it('should revert if not called by owner', async () => { const expectedError = new OwnableRevertErrors.OnlyOwnerError(notOwner, owner); const tx = authorizable.addAuthorizedAddress.sendTransactionAsync(notOwner, { from: notOwner }); return expect(tx).to.revertWith(expectedError); @@ -67,28 +65,26 @@ describe('Authorizable', () => { expect(isAuthorized).to.be.true(); }); - it('should throw if owner attempts to authorize the zero address', async () => { - return expectTransactionFailedAsync( - authorizable.addAuthorizedAddress.sendTransactionAsync(constants.NULL_ADDRESS, { from: owner }), - RevertReason.ZeroCantBeAuthorized, - ); + it('should revert if owner attempts to authorize the zero address', async () => { + const expectedError = new AuthorizableRevertErrors.ZeroCantBeAuthorizedError(); + const tx = authorizable.addAuthorizedAddress.sendTransactionAsync(constants.NULL_ADDRESS, { from: owner }); + return expect(tx).to.revertWith(expectedError); }); - it('should throw if owner attempts to authorize a duplicate address', async () => { + it('should revert if owner attempts to authorize a duplicate address', async () => { await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( address, { from: owner }, constants.AWAIT_TRANSACTION_MINED_MS, ); - return expectTransactionFailedAsync( - authorizable.addAuthorizedAddress.sendTransactionAsync(address, { from: owner }), - RevertReason.TargetAlreadyAuthorized, - ); + const expectedError = new AuthorizableRevertErrors.TargetAlreadyAuthorizedError(address); + const tx = authorizable.addAuthorizedAddress.sendTransactionAsync(address, { from: owner }); + return expect(tx).to.revertWith(expectedError); }); }); describe('removeAuthorizedAddress', () => { - it('should throw if not called by owner', async () => { + it('should revert if not called by owner', async () => { await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( address, { from: owner }, @@ -114,18 +110,15 @@ describe('Authorizable', () => { expect(isAuthorized).to.be.false(); }); - it('should throw if owner attempts to remove an address that is not authorized', async () => { - return expectTransactionFailedAsync( - authorizable.removeAuthorizedAddress.sendTransactionAsync(address, { - from: owner, - }), - RevertReason.TargetNotAuthorized, - ); + it('should revert if owner attempts to remove an address that is not authorized', async () => { + const expectedError = new AuthorizableRevertErrors.TargetNotAuthorizedError(address); + const tx = authorizable.removeAuthorizedAddress.sendTransactionAsync(address, { from: owner }); + return expect(tx).to.revertWith(expectedError); }); }); describe('removeAuthorizedAddressAtIndex', () => { - it('should throw if not called by owner', async () => { + it('should revert if not called by owner', async () => { await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( address, { from: owner }, @@ -139,32 +132,26 @@ describe('Authorizable', () => { return expect(tx).to.revertWith(expectedError); }); - it('should throw if index is >= authorities.length', async () => { + it('should revert if index is >= authorities.length', async () => { await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( address, { from: owner }, constants.AWAIT_TRANSACTION_MINED_MS, ); const index = new BigNumber(1); - return expectTransactionFailedAsync( - authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, { - from: owner, - }), - RevertReason.IndexOutOfBounds, - ); + const expectedError = new AuthorizableRevertErrors.IndexOutOfBoundsError(index, constants.ZERO_AMOUNT); + const tx = authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, { from: owner }); + return expect(tx).to.revertWith(expectedError); }); - it('should throw if owner attempts to remove an address that is not authorized', async () => { + it('should revert if owner attempts to remove an address that is not authorized', async () => { const index = new BigNumber(0); - return expectTransactionFailedAsync( - authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, { - from: owner, - }), - RevertReason.TargetNotAuthorized, - ); + const expectedError = new AuthorizableRevertErrors.TargetNotAuthorizedError(address); + const tx = authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, { from: owner }); + return expect(tx).to.revertWith(expectedError); }); - it('should throw if address at index does not match target', async () => { + it('should revert if address at index does not match target', async () => { const address1 = address; const address2 = notOwner; await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( @@ -178,12 +165,9 @@ describe('Authorizable', () => { constants.AWAIT_TRANSACTION_MINED_MS, ); const address1Index = new BigNumber(0); - return expectTransactionFailedAsync( - authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address2, address1Index, { - from: owner, - }), - RevertReason.AuthorizedAddressMismatch, - ); + const expectedError = new AuthorizableRevertErrors.AuthorizedAddressMismatchError(address1, address2); + const tx = authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address2, address1Index, { from: owner }); + return expect(tx).to.revertWith(expectedError); }); it('should allow owner to remove an authorized address', async () => { diff --git a/contracts/utils/tsconfig.json b/contracts/utils/tsconfig.json index 8d08dbc532..35e669ea77 100644 --- a/contracts/utils/tsconfig.json +++ b/contracts/utils/tsconfig.json @@ -4,6 +4,7 @@ "include": ["./src/**/*", "./test/**/*", "./generated-wrappers/**/*"], "files": [ "generated-artifacts/Authorizable.json", + "generated-artifacts/IAuthorizable.json", "generated-artifacts/IOwnable.json", "generated-artifacts/LibAddress.json", "generated-artifacts/LibBytes.json", diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 1a5e9c8d35..57ce884d27 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -268,7 +268,6 @@ export enum RevertReason { SenderNotAuthorized = 'SENDER_NOT_AUTHORIZED', TargetNotAuthorized = 'TARGET_NOT_AUTHORIZED', TargetAlreadyAuthorized = 'TARGET_ALREADY_AUTHORIZED', - ZeroCantBeAuthorized = 'ZERO_CANT_BE_AUTHORIZED', IndexOutOfBounds = 'INDEX_OUT_OF_BOUNDS', AuthorizedAddressMismatch = 'AUTHORIZED_ADDRESS_MISMATCH', OnlyContractOwner = 'ONLY_CONTRACT_OWNER', diff --git a/packages/utils/src/authorizable_revert_errors.ts b/packages/utils/src/authorizable_revert_errors.ts new file mode 100644 index 0000000000..81dce1eaf1 --- /dev/null +++ b/packages/utils/src/authorizable_revert_errors.ts @@ -0,0 +1,69 @@ +import { BigNumber } from './configured_bignumber'; +import { RevertError } from './revert_error'; + +// tslint:disable:max-classes-per-file +export class AuthorizedAddressMismatchError extends RevertError { + constructor(authorized?: string, target?: string) { + super( + 'AuthorizedAddressMismatchError', + 'AuthorizedAddressMismatchError(address authorized, address target)', + { authorized, target }, + ); + } +} + +export class IndexOutOfBoundsError extends RevertError { + constructor(index?: BigNumber, length?: BigNumber) { + super( + 'IndexOutOfBoundsError', + 'IndexOutOfBoundsError(uint256 index, uint256 length)', + { index, length }, + ); + } +} + +export class SenderNotAuthorizedError extends RevertError { + constructor(sender?: string) { + super('SenderNotAuthorizedError', 'SenderNotAuthorizedError()', { sender }); + } +} + +export class TargetAlreadyAuthorizedError extends RevertError { + constructor(target?: string) { + super( + 'TargetAlreadyAuthorizedError', + 'TargetAlreadyAuthorizedError(address target)', + { target }, + ); + } +} + +export class TargetNotAuthorizedError extends RevertError { + constructor(target?: string) { + super( + 'TargetNotAuthorizedError', + 'TargetNotAuthorizedError(address target)', + { target }, + ); + } +} + +export class ZeroCantBeAuthorizedError extends RevertError { + constructor() { + super('ZeroCantBeAuthorizedError', 'ZeroCantBeAuthorizedError()', {}); + } +} + +const types = [ + AuthorizedAddressMismatchError, + IndexOutOfBoundsError, + SenderNotAuthorizedError, + TargetAlreadyAuthorizedError, + TargetNotAuthorizedError, + ZeroCantBeAuthorizedError, +]; + +// Register the types we've defined. +for (const type of types) { + RevertError.registerType(type); +} diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index a9c392d577..7ecb00b4f9 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -1,3 +1,4 @@ +import * as AuthorizableRevertErrors from './authorizable_revert_errors'; import * as LibAddressArrayRevertErrors from './lib_address_array_revert_errors'; import * as LibBytesRevertErrors from './lib_bytes_revert_errors'; import * as OwnableRevertErrors from './ownable_revert_errors'; @@ -32,6 +33,7 @@ export { } from './revert_error'; export { + AuthorizableRevertErrors, LibAddressArrayRevertErrors, LibBytesRevertErrors, OwnableRevertErrors,