From 21ff95450f743ce89aa3fbf07a501c9ef4322367 Mon Sep 17 00:00:00 2001 From: Jan-Felix Date: Fri, 14 Apr 2023 13:33:50 +0200 Subject: [PATCH 01/13] fix Operator enum comments --- packages/evm/contracts/Types.sol | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/evm/contracts/Types.sol b/packages/evm/contracts/Types.sol index 5ae21d9dd..f62fc4990 100644 --- a/packages/evm/contracts/Types.sol +++ b/packages/evm/contracts/Types.sol @@ -19,8 +19,8 @@ enum ParameterType { enum Operator { // 00: EMPTY EXPRESSION (default, always passes) // paramType: Static / Dynamic / Tuple / Array - // 🚫 children // ❓ children (only for paramType: Tuple / Array to describe their structure) + // 🚫 compValue /* 00: */ Pass, // ------------------------------------------------------------ // 01-04: LOGICAL EXPRESSIONS @@ -32,7 +32,7 @@ enum Operator { /* 03: */ Nor, /* 04: */ Xor, // ------------------------------------------------------------ - // 05-16: COMPLEX EXPRESSIONS + // 05-14: COMPLEX EXPRESSIONS // paramType: AbiEncoded / Tuple / Array, // βœ… children // 🚫 compValue @@ -46,6 +46,11 @@ enum Operator { /* 12: */ _Placeholder12, /* 13: */ _Placeholder13, /* 14: */ _Placeholder14, + // ------------------------------------------------------------ + // 15: SPECIAL COMPARISON (without compValue) + // paramType: Static + // 🚫 children + // 🚫 compValue /* 15: */ EqualToAvatar, // ------------------------------------------------------------ // 16-31: COMPARISON EXPRESSIONS From 51a5393a6317bef21abad833240e63f6c8b0d3b1 Mon Sep 17 00:00:00 2001 From: Jan-Felix Date: Mon, 17 Apr 2023 13:35:37 +0200 Subject: [PATCH 02/13] add missing conditions param natspec --- packages/evm/contracts/PermissionBuilder.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/evm/contracts/PermissionBuilder.sol b/packages/evm/contracts/PermissionBuilder.sol index 7ad56ddec..b2dd24e7e 100644 --- a/packages/evm/contracts/PermissionBuilder.sol +++ b/packages/evm/contracts/PermissionBuilder.sol @@ -125,10 +125,11 @@ abstract contract PermissionBuilder is Core { emit RevokeFunction(roleKey, targetAddress, selector); } - /// @dev Defines the values that can be called for a given function for each param. + /// @dev Sets conditions to enforce on calls to the specified target. /// @param roleKey identifier of the role to be modified. /// @param targetAddress Destination address of transaction. /// @param selector 4 byte function selector. + /// @param conditions The conditions to enforce. /// @param options designates if a transaction can send ether and/or delegatecall to target. function scopeFunction( bytes32 roleKey, From 632662f8cd82cd5223a354426560f0e7f7f36e90 Mon Sep 17 00:00:00 2001 From: Jan-Felix Date: Mon, 17 Apr 2023 14:56:31 +0200 Subject: [PATCH 03/13] fix code comment --- packages/evm/contracts/WriteOnce.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/evm/contracts/WriteOnce.sol b/packages/evm/contracts/WriteOnce.sol index 71e67366e..8578491fd 100644 --- a/packages/evm/contracts/WriteOnce.sol +++ b/packages/evm/contracts/WriteOnce.sol @@ -109,7 +109,7 @@ library WriteOnce { hex"63", uint32(data.length + 1), hex"80_60_0E_60_00_39_60_00_F3", - // Append 00 to data so contract can't be called + // Prepend 00 to data so contract can't be called hex"00", data ); From a11e2e585f18e3e8f6b04075e7b5a0fd72fe822f Mon Sep 17 00:00:00 2001 From: Jan-Felix Date: Thu, 20 Apr 2023 15:33:19 +0200 Subject: [PATCH 04/13] also fix comment in test utils --- packages/evm/test/utils.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/evm/test/utils.ts b/packages/evm/test/utils.ts index 8e31dfec0..04f062c67 100644 --- a/packages/evm/test/utils.ts +++ b/packages/evm/test/utils.ts @@ -125,8 +125,8 @@ export enum ParameterType { export enum Operator { // 00: EMPTY EXPRESSION (default, always passes) // paramType: Static / Dynamic / Tuple / Array - // 🚫 children // ❓ children (only for paramType: Tuple / Array to describe their structure) + // 🚫 compValue /* 00: */ Pass = 0, // ------------------------------------------------------------ // 01-04: LOGICAL EXPRESSIONS @@ -138,7 +138,7 @@ export enum Operator { /* 03: */ Nor, /* 04: */ Xor, // ------------------------------------------------------------ - // 05-16: COMPLEX EXPRESSIONS + // 05-14: COMPLEX EXPRESSIONS // paramType: AbiEncoded / Tuple / Array, // βœ… children // 🚫 compValue @@ -152,6 +152,11 @@ export enum Operator { /* 12: */ _Placeholder12, /* 13: */ _Placeholder13, /* 14: */ _Placeholder14, + // ------------------------------------------------------------ + // 15: SPECIAL COMPARISON (without compValue) + // paramType: Static + // 🚫 children + // 🚫 compValue /* 15: */ EqualToAvatar, // ------------------------------------------------------------ // 16-31: COMPARISON EXPRESSIONS From ebad4642ea6e2ac5f19d4d55ffa0291999072945 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Sun, 23 Apr 2023 10:54:24 +0800 Subject: [PATCH 05/13] Only validating type trees when there's more than one child, for gas savings --- packages/evm/contracts/Integrity.sol | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/evm/contracts/Integrity.sol b/packages/evm/contracts/Integrity.sol index 9c68bc749..5ba2a9086 100644 --- a/packages/evm/contracts/Integrity.sol +++ b/packages/evm/contracts/Integrity.sol @@ -230,9 +230,10 @@ library Integrity { for (uint256 i = 0; i < conditions.length; i++) { ConditionFlat memory condition = conditions[i]; if ( - (condition.operator >= Operator.And && + ((condition.operator >= Operator.And && condition.operator <= Operator.Nor) || - condition.paramType == ParameterType.Array + condition.paramType == ParameterType.Array) && + childrenBounds[i].length > 1 ) { compatiblechildTypeTree(conditions, i, childrenBounds); } @@ -254,8 +255,6 @@ library Integrity { uint256 index, Topology.Bounds[] memory childrenBounds ) private pure { - assert(childrenBounds[index].length > 0); - uint256 start = childrenBounds[index].start; uint256 end = childrenBounds[index].end; From 73443d70554aea20d3758c7287f0ea93eb7e42d3 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Sun, 23 Apr 2023 11:08:01 +0800 Subject: [PATCH 06/13] Caching storage entry during flush, for gas savings --- packages/evm/contracts/AllowanceTracker.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/evm/contracts/AllowanceTracker.sol b/packages/evm/contracts/AllowanceTracker.sol index 1fd3b3a57..45a0c17da 100644 --- a/packages/evm/contracts/AllowanceTracker.sol +++ b/packages/evm/contracts/AllowanceTracker.sol @@ -62,7 +62,7 @@ abstract contract AllowanceTracker is Core { // Retrieve the allowance and calculate its current updated balance // and next refill timestamp. - Allowance memory allowance = allowances[key]; + Allowance storage allowance = allowances[key]; (uint128 balance, uint64 refillTimestamp) = _accruedAllowance( allowance, block.timestamp @@ -71,8 +71,8 @@ abstract contract AllowanceTracker is Core { assert(balance == consumption.balance); assert(consumed <= balance); // Flush - allowances[key].balance = balance - consumed; - allowances[key].refillTimestamp = refillTimestamp; + allowance.balance = balance - consumed; + allowance.refillTimestamp = refillTimestamp; // Emit an event to signal the total consumed amount. emit ConsumeAllowance(key, consumed, balance - consumed); From 402717814e83cb6cad48dbafce72ffb911dafb32 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Sun, 23 Apr 2023 11:33:40 +0800 Subject: [PATCH 07/13] Make setup more efficient by calling a more lightweight version of _transferOwnership --- packages/evm/contracts/Roles.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/evm/contracts/Roles.sol b/packages/evm/contracts/Roles.sol index bce7b8160..8b47c7d5c 100644 --- a/packages/evm/contracts/Roles.sol +++ b/packages/evm/contracts/Roles.sol @@ -58,7 +58,7 @@ contract Roles is avatar = _avatar; target = _target; - transferOwnership(_owner); + _transferOwnership(_owner); setupModules(); emit RolesModSetup(msg.sender, _owner, _avatar, _target); From f9283fdacaa28547c313d6c15df58d5944fd3f7b Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Sun, 23 Apr 2023 11:43:47 +0800 Subject: [PATCH 08/13] Introducing UnsupportedOperator custom error for the Integrity flow --- packages/evm/contracts/Integrity.sol | 15 ++++++++------- packages/evm/test/Integrity.spec.ts | 8 +++++--- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/evm/contracts/Integrity.sol b/packages/evm/contracts/Integrity.sol index 5ba2a9086..9af1673a4 100644 --- a/packages/evm/contracts/Integrity.sol +++ b/packages/evm/contracts/Integrity.sol @@ -17,6 +17,8 @@ library Integrity { error UnsuitableCompValue(uint256 index); + error UnsupportedOperator(uint256 index); + error UnsuitableParent(uint256 index); error UnsuitableChildCount(uint256 index); @@ -131,19 +133,18 @@ library Integrity { if (compValue.length != 32) { revert UnsuitableCompValue(index); } - } else { - require( - operator == Operator.EtherWithinAllowance || - operator == Operator.CallWithinAllowance, - "Placeholder Operators are not supported" - ); - + } else if ( + operator == Operator.EtherWithinAllowance || + operator == Operator.CallWithinAllowance + ) { if (paramType != ParameterType.None) { revert UnsuitableParameterType(index); } if (compValue.length != 32) { revert UnsuitableCompValue(index); } + } else { + revert UnsupportedOperator(index); } } diff --git a/packages/evm/test/Integrity.spec.ts b/packages/evm/test/Integrity.spec.ts index a5cdcc1c8..744d8e541 100644 --- a/packages/evm/test/Integrity.spec.ts +++ b/packages/evm/test/Integrity.spec.ts @@ -690,8 +690,8 @@ describe("Integrity", async () => { ]) ).to.not.be.reverted; }); - it("Not possible to setup a node with Operator Placeholder", async () => { - const { enforce } = await loadFixture(setup); + it("Node *Placeholder* triggers error", async () => { + const { integrity, enforce } = await loadFixture(setup); await expect( enforce([ { @@ -707,7 +707,9 @@ describe("Integrity", async () => { compValue: "0x", }, ]) - ).to.be.revertedWith("Placeholder Operators are not supported"); + ) + .to.be.revertedWithCustomError(integrity, "UnsupportedOperator") + .withArgs(1); }); }); From e4d45382096aefb9f207c67d47cc8c2c27c15774 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Mon, 1 May 2023 11:11:33 +0800 Subject: [PATCH 09/13] Using key based structure style for TargetAddress instance creation --- packages/evm/contracts/PermissionBuilder.sol | 24 ++++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/evm/contracts/PermissionBuilder.sol b/packages/evm/contracts/PermissionBuilder.sol index b2dd24e7e..26f2cc858 100644 --- a/packages/evm/contracts/PermissionBuilder.sol +++ b/packages/evm/contracts/PermissionBuilder.sol @@ -60,10 +60,10 @@ abstract contract PermissionBuilder is Core { address targetAddress, ExecutionOptions options ) external onlyOwner { - roles[roleKey].targets[targetAddress] = TargetAddress( - Clearance.Target, - options - ); + roles[roleKey].targets[targetAddress] = TargetAddress({ + clearance: Clearance.Target, + options: options + }); emit AllowTarget(roleKey, targetAddress, options); } @@ -74,10 +74,10 @@ abstract contract PermissionBuilder is Core { bytes32 roleKey, address targetAddress ) external onlyOwner { - roles[roleKey].targets[targetAddress] = TargetAddress( - Clearance.None, - ExecutionOptions.None - ); + roles[roleKey].targets[targetAddress] = TargetAddress({ + clearance: Clearance.None, + options: ExecutionOptions.None + }); emit RevokeTarget(roleKey, targetAddress); } @@ -88,10 +88,10 @@ abstract contract PermissionBuilder is Core { bytes32 roleKey, address targetAddress ) external onlyOwner { - roles[roleKey].targets[targetAddress] = TargetAddress( - Clearance.Function, - ExecutionOptions.None - ); + roles[roleKey].targets[targetAddress] = TargetAddress({ + clearance: Clearance.Function, + options: ExecutionOptions.None + }); emit ScopeTarget(roleKey, targetAddress); } From 08349bfc5cd84fb490586bfff417a2053cbc18ca Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Mon, 1 May 2023 11:43:59 +0800 Subject: [PATCH 10/13] Be more selective with the use of unchecked --- packages/evm/contracts/AllowanceTracker.sol | 68 ++--- packages/evm/contracts/Consumptions.sol | 65 +++-- packages/evm/contracts/Decoder.sol | 28 +- packages/evm/contracts/PermissionChecker.sol | 250 +++++++++--------- packages/evm/contracts/PermissionLoader.sol | 100 +++---- packages/evm/contracts/Topology.sol | 78 +++--- packages/evm/contracts/WriteOnce.sol | 26 +- .../evm/contracts/packers/BufferPacker.sol | 47 ++-- packages/evm/contracts/packers/Packer.sol | 23 +- 9 files changed, 359 insertions(+), 326 deletions(-) diff --git a/packages/evm/contracts/AllowanceTracker.sol b/packages/evm/contracts/AllowanceTracker.sol index 45a0c17da..42b041a4f 100644 --- a/packages/evm/contracts/AllowanceTracker.sol +++ b/packages/evm/contracts/AllowanceTracker.sol @@ -53,29 +53,32 @@ abstract contract AllowanceTracker is Core { */ function _flushPrepare(Consumption[] memory consumptions) internal { uint256 count = consumptions.length; - unchecked { - for (uint256 i; i < count; ++i) { - Consumption memory consumption = consumptions[i]; - bytes32 key = consumption.allowanceKey; - uint128 consumed = consumption.consumed; + for (uint256 i; i < count; ) { + Consumption memory consumption = consumptions[i]; - // Retrieve the allowance and calculate its current updated balance - // and next refill timestamp. - Allowance storage allowance = allowances[key]; - (uint128 balance, uint64 refillTimestamp) = _accruedAllowance( - allowance, - block.timestamp - ); + bytes32 key = consumption.allowanceKey; + uint128 consumed = consumption.consumed; + + // Retrieve the allowance and calculate its current updated balance + // and next refill timestamp. + Allowance storage allowance = allowances[key]; + (uint128 balance, uint64 refillTimestamp) = _accruedAllowance( + allowance, + block.timestamp + ); + + assert(balance == consumption.balance); + assert(consumed <= balance); + // Flush + allowance.balance = balance - consumed; + allowance.refillTimestamp = refillTimestamp; - assert(balance == consumption.balance); - assert(consumed <= balance); - // Flush - allowance.balance = balance - consumed; - allowance.refillTimestamp = refillTimestamp; + // Emit an event to signal the total consumed amount. + emit ConsumeAllowance(key, consumed, balance - consumed); - // Emit an event to signal the total consumed amount. - emit ConsumeAllowance(key, consumed, balance - consumed); + unchecked { + ++i; } } } @@ -93,19 +96,20 @@ abstract contract AllowanceTracker is Core { bool success ) internal { uint256 count = consumptions.length; - unchecked { - for (uint256 i; i < count; ++i) { - Consumption memory consumption = consumptions[i]; - bytes32 key = consumption.allowanceKey; - if (success) { - emit ConsumeAllowance( - key, - consumption.consumed, - consumption.balance - consumption.consumed - ); - } else { - allowances[key].balance = consumption.balance; - } + for (uint256 i; i < count; ) { + Consumption memory consumption = consumptions[i]; + bytes32 key = consumption.allowanceKey; + if (success) { + emit ConsumeAllowance( + key, + consumption.consumed, + consumption.balance - consumption.consumed + ); + } else { + allowances[key].balance = consumption.balance; + } + unchecked { + ++i; } } } diff --git a/packages/evm/contracts/Consumptions.sol b/packages/evm/contracts/Consumptions.sol index cd8261997..e9c5776d5 100644 --- a/packages/evm/contracts/Consumptions.sol +++ b/packages/evm/contracts/Consumptions.sol @@ -13,12 +13,15 @@ library Consumptions { Consumption[] memory consumptions ) internal pure returns (Consumption[] memory result) { uint256 length = consumptions.length; - unchecked { - result = new Consumption[](length); - for (uint256 i; i < length; ++i) { - result[i].allowanceKey = consumptions[i].allowanceKey; - result[i].balance = consumptions[i].balance; - result[i].consumed = consumptions[i].consumed; + + result = new Consumption[](length); + for (uint256 i; i < length; ) { + result[i].allowanceKey = consumptions[i].allowanceKey; + result[i].balance = consumptions[i].balance; + result[i].consumed = consumptions[i].consumed; + + unchecked { + ++i; } } } @@ -28,11 +31,14 @@ library Consumptions { bytes32 key ) internal pure returns (uint256, bool) { uint256 length = consumptions.length; - unchecked { - for (uint256 i; i < length; ++i) { - if (consumptions[i].allowanceKey == key) { - return (i, true); - } + + for (uint256 i; i < length; ) { + if (consumptions[i].allowanceKey == key) { + return (i, true); + } + + unchecked { + ++i; } } @@ -49,23 +55,30 @@ library Consumptions { result = new Consumption[](c1.length + c2.length); uint256 length = c1.length; - unchecked { - for (uint256 i; i < length; ++i) { - result[i].allowanceKey = c1[i].allowanceKey; - result[i].balance = c1[i].balance; - result[i].consumed = c1[i].consumed; + + for (uint256 i; i < length; ) { + result[i].allowanceKey = c1[i].allowanceKey; + result[i].balance = c1[i].balance; + result[i].consumed = c1[i].consumed; + + unchecked { + ++i; + } + } + + for (uint256 i; i < c2.length; ) { + (uint256 index, bool found) = find(c1, c2[i].allowanceKey); + if (found) { + result[index].consumed += c2[i].consumed; + } else { + result[length].allowanceKey = c2[i].allowanceKey; + result[length].balance = c2[i].balance; + result[length].consumed = c2[i].consumed; + length++; } - for (uint256 i; i < c2.length; ++i) { - (uint256 index, bool found) = find(c1, c2[i].allowanceKey); - if (found) { - result[index].consumed += c2[i].consumed; - } else { - result[length].allowanceKey = c2[i].allowanceKey; - result[length].balance = c2[i].balance; - result[length].consumed = c2[i].consumed; - length++; - } + unchecked { + ++i; } } diff --git a/packages/evm/contracts/Decoder.sol b/packages/evm/contracts/Decoder.sol index bd9f570ee..02018926a 100644 --- a/packages/evm/contracts/Decoder.sol +++ b/packages/evm/contracts/Decoder.sol @@ -111,21 +111,23 @@ library Decoder { bool isInline; if (template) isInline = Topology.isInline(node.children[0]); - unchecked { - uint256 offset; - for (uint256 i; i < length; ++i) { - if (!template) isInline = Topology.isInline(node.children[i]); + uint256 offset; + for (uint256 i; i < length; ) { + if (!template) isInline = Topology.isInline(node.children[i]); - _walk( - data, - _locationInBlock(data, location, offset, isInline), - node.children[template ? 0 : i], - result.children[i] - ); + _walk( + data, + _locationInBlock(data, location, offset, isInline), + node.children[template ? 0 : i], + result.children[i] + ); + + uint256 childSize = result.children[i].size; + result.size += isInline ? childSize : childSize + 32; + offset += isInline ? childSize : 32; - uint256 childSize = result.children[i].size; - result.size += isInline ? childSize : childSize + 32; - offset += isInline ? childSize : 32; + unchecked { + ++i; } } } diff --git a/packages/evm/contracts/PermissionChecker.sol b/packages/evm/contracts/PermissionChecker.sol index 35ca9f1eb..03cfc233c 100644 --- a/packages/evm/contracts/PermissionChecker.sol +++ b/packages/evm/contracts/PermissionChecker.sol @@ -80,22 +80,23 @@ abstract contract PermissionChecker is Core, Periphery { try adapter.unwrap(to, value, data, operation) returns ( UnwrappedTransaction[] memory transactions ) { - unchecked { - for (uint256 i; i < transactions.length; ++i) { - UnwrappedTransaction memory transaction = transactions[i]; - uint256 left = transaction.dataLocation; - uint256 right = left + transaction.dataSize; - (status, result) = _transaction( - role, - transaction.to, - transaction.value, - data[left:right], - transaction.operation, - result.consumptions - ); - if (status != Status.Ok) { - return (status, result); - } + for (uint256 i; i < transactions.length; ) { + UnwrappedTransaction memory transaction = transactions[i]; + uint256 left = transaction.dataLocation; + uint256 right = left + transaction.dataSize; + (status, result) = _transaction( + role, + transaction.to, + transaction.value, + data[left:right], + transaction.operation, + result.consumptions + ); + if (status != Status.Ok) { + return (status, result); + } + unchecked { + ++i; } } } catch { @@ -296,21 +297,22 @@ abstract contract PermissionChecker is Core, Periphery { return (Status.ParameterNotAMatch, result); } - unchecked { - for (uint256 i; i < condition.children.length; ++i) { - (status, result) = _walk( - value, - data, - condition.children[i], - payload.children[i], - result.consumptions + for (uint256 i; i < condition.children.length; ) { + (status, result) = _walk( + value, + data, + condition.children[i], + payload.children[i], + result.consumptions + ); + if (status != Status.Ok) { + return ( + status, + Result({consumptions: consumptions, info: result.info}) ); - if (status != Status.Ok) { - return ( - status, - Result({consumptions: consumptions, info: result.info}) - ); - } + } + unchecked { + ++i; } } @@ -326,21 +328,22 @@ abstract contract PermissionChecker is Core, Periphery { ) private pure returns (Status status, Result memory result) { result = Result({consumptions: consumptions, info: 0}); - unchecked { - for (uint256 i; i < condition.children.length; ++i) { - (status, result) = _walk( - value, - data, - condition.children[i], - payload, - result.consumptions + for (uint256 i; i < condition.children.length; ) { + (status, result) = _walk( + value, + data, + condition.children[i], + payload, + result.consumptions + ); + if (status != Status.Ok) { + return ( + status, + Result({consumptions: consumptions, info: result.info}) ); - if (status != Status.Ok) { - return ( - status, - Result({consumptions: consumptions, info: result.info}) - ); - } + } + unchecked { + ++i; } } return (Status.Ok, result); @@ -353,18 +356,19 @@ abstract contract PermissionChecker is Core, Periphery { ParameterPayload memory payload, Consumption[] memory consumptions ) private pure returns (Status, Result memory) { - unchecked { - for (uint256 i; i < condition.children.length; ++i) { - (Status status, Result memory result) = _walk( - value, - data, - condition.children[i], - payload, - consumptions - ); - if (status == Status.Ok) { - return (status, result); - } + for (uint256 i; i < condition.children.length; ) { + (Status status, Result memory result) = _walk( + value, + data, + condition.children[i], + payload, + consumptions + ); + if (status == Status.Ok) { + return (status, result); + } + unchecked { + ++i; } } @@ -381,21 +385,22 @@ abstract contract PermissionChecker is Core, Periphery { ParameterPayload memory payload, Consumption[] memory consumptions ) private pure returns (Status, Result memory) { - unchecked { - for (uint256 i; i < condition.children.length; ++i) { - (Status status, ) = _walk( - value, - data, - condition.children[i], - payload, - consumptions + for (uint256 i; i < condition.children.length; ) { + (Status status, ) = _walk( + value, + data, + condition.children[i], + payload, + consumptions + ); + if (status == Status.Ok) { + return ( + Status.NorViolation, + Result({consumptions: consumptions, info: 0}) ); - if (status == Status.Ok) { - return ( - Status.NorViolation, - Result({consumptions: consumptions, info: 0}) - ); - } + } + unchecked { + ++i; } } return (Status.Ok, Result({consumptions: consumptions, info: 0})); @@ -408,18 +413,19 @@ abstract contract PermissionChecker is Core, Periphery { ParameterPayload memory payload, Consumption[] memory consumptions ) private pure returns (Status, Result memory) { - unchecked { - for (uint256 i; i < payload.children.length; ++i) { - (Status status, Result memory result) = _walk( - value, - data, - condition.children[0], - payload.children[i], - consumptions - ); - if (status == Status.Ok) { - return (status, result); - } + for (uint256 i; i < payload.children.length; ) { + (Status status, Result memory result) = _walk( + value, + data, + condition.children[0], + payload.children[i], + consumptions + ); + if (status == Status.Ok) { + return (status, result); + } + unchecked { + ++i; } } return ( @@ -436,21 +442,22 @@ abstract contract PermissionChecker is Core, Periphery { Consumption[] memory consumptions ) private pure returns (Status status, Result memory result) { result = Result({consumptions: consumptions, info: 0}); - unchecked { - for (uint256 i; i < payload.children.length; ++i) { - (status, result) = _walk( - value, - data, - condition.children[0], - payload.children[i], - result.consumptions + for (uint256 i; i < payload.children.length; ) { + (status, result) = _walk( + value, + data, + condition.children[0], + payload.children[i], + result.consumptions + ); + if (status != Status.Ok) { + return ( + Status.NotEveryArrayElementPasses, + Result({consumptions: consumptions, info: 0}) ); - if (status != Status.Ok) { - return ( - Status.NotEveryArrayElementPasses, - Result({consumptions: consumptions, info: 0}) - ); - } + } + unchecked { + ++i; } } return (Status.Ok, result); @@ -472,35 +479,34 @@ abstract contract PermissionChecker is Core, Periphery { return (Status.ParameterNotSubsetOfAllowed, result); } - unchecked { - uint256 taken; - for (uint256 i; i < payload.children.length; ++i) { - bool found = false; - for (uint256 j; j < condition.children.length; ++j) { - if (taken & (1 << j) != 0) continue; - - (Status status, Result memory _result) = _walk( - value, - data, - condition.children[j], - payload.children[i], - result.consumptions - ); - if (status == Status.Ok) { - found = true; - taken |= 1 << j; - result = _result; - break; - } - } - if (!found) { - return ( - Status.ParameterNotSubsetOfAllowed, - Result({consumptions: consumptions, info: 0}) - ); + uint256 taken; + for (uint256 i; i < payload.children.length; ++i) { + bool found = false; + for (uint256 j; j < condition.children.length; ++j) { + if (taken & (1 << j) != 0) continue; + + (Status status, Result memory _result) = _walk( + value, + data, + condition.children[j], + payload.children[i], + result.consumptions + ); + if (status == Status.Ok) { + found = true; + taken |= 1 << j; + result = _result; + break; } } + if (!found) { + return ( + Status.ParameterNotSubsetOfAllowed, + Result({consumptions: consumptions, info: 0}) + ); + } } + return (Status.Ok, result); } diff --git a/packages/evm/contracts/PermissionLoader.sol b/packages/evm/contracts/PermissionLoader.sol index c46e4cb87..f3bbe7a16 100644 --- a/packages/evm/contracts/PermissionLoader.sol +++ b/packages/evm/contracts/PermissionLoader.sol @@ -51,17 +51,19 @@ abstract contract PermissionLoader is Core { ) = BufferPacker.unpackBody(buffer, count); uint256 allowanceCount; - unchecked { - for (uint256 i; i < conditionsFlat.length; ++i) { - Operator operator = conditionsFlat[i].operator; - if (operator >= Operator.WithinAllowance) { - ++allowanceCount; - } else if (operator == Operator.EqualToAvatar) { - // patch Operator.EqualToAvatar which in reality works as - // a placeholder - conditionsFlat[i].operator = Operator.EqualTo; - compValues[i] = keccak256(abi.encode(avatar)); - } + + for (uint256 i; i < conditionsFlat.length; ) { + Operator operator = conditionsFlat[i].operator; + if (operator >= Operator.WithinAllowance) { + ++allowanceCount; + } else if (operator == Operator.EqualToAvatar) { + // patch Operator.EqualToAvatar which in reality works as + // a placeholder + conditionsFlat[i].operator = Operator.EqualTo; + compValues[i] = keccak256(abi.encode(avatar)); + } + unchecked { + ++i; } } @@ -91,28 +93,29 @@ abstract contract PermissionLoader is Core { // This function populates a buffer received as an argument instead of // instantiating a result object. This is an important gas optimization - unchecked { - ConditionFlat memory conditionFlat = conditionsFlat[index]; - treeNode.paramType = conditionFlat.paramType; - treeNode.operator = conditionFlat.operator; - treeNode.compValue = compValues[index]; + ConditionFlat memory conditionFlat = conditionsFlat[index]; + treeNode.paramType = conditionFlat.paramType; + treeNode.operator = conditionFlat.operator; + treeNode.compValue = compValues[index]; - if (childrenBounds[index].length == 0) { - return; - } + if (childrenBounds[index].length == 0) { + return; + } - uint256 start = childrenBounds[index].start; - uint256 length = childrenBounds[index].length; - - treeNode.children = new Condition[](length); - for (uint j; j < length; ++j) { - _conditionTree( - conditionsFlat, - compValues, - childrenBounds, - start + j, - treeNode.children[j] - ); + uint256 start = childrenBounds[index].start; + uint256 length = childrenBounds[index].length; + + treeNode.children = new Condition[](length); + for (uint j; j < length; ) { + _conditionTree( + conditionsFlat, + compValues, + childrenBounds, + start + j, + treeNode.children[j] + ); + unchecked { + ++j; } } } @@ -126,25 +129,24 @@ abstract contract PermissionLoader is Core { result = new Consumption[](maxAllowanceCount); uint256 insert; - unchecked { - for (uint256 i; i < count; ++i) { - if (conditions[i].operator < Operator.WithinAllowance) { - continue; - } - - bytes32 key = compValues[i]; - (, bool contains) = Consumptions.find(result, key); - if (contains) { - continue; - } - - result[insert].allowanceKey = key; - (result[insert].balance, ) = _accruedAllowance( - allowances[key], - block.timestamp - ); - insert++; + + for (uint256 i; i < count; ++i) { + if (conditions[i].operator < Operator.WithinAllowance) { + continue; + } + + bytes32 key = compValues[i]; + (, bool contains) = Consumptions.find(result, key); + if (contains) { + continue; } + + result[insert].allowanceKey = key; + (result[insert].balance, ) = _accruedAllowance( + allowances[key], + block.timestamp + ); + insert++; } if (insert < maxAllowanceCount) { diff --git a/packages/evm/contracts/Topology.sol b/packages/evm/contracts/Topology.sol index b7ccadd5e..1ca3bba86 100644 --- a/packages/evm/contracts/Topology.sol +++ b/packages/evm/contracts/Topology.sol @@ -26,20 +26,21 @@ library Topology { uint256 count = conditions.length; assert(count > 0); - unchecked { - // parents are breadth-first - result = new Bounds[](count); - result[0].start = type(uint256).max; + // parents are breadth-first + result = new Bounds[](count); + result[0].start = type(uint256).max; - // first item is the root - for (uint256 i = 1; i < count; ++i) { - result[i].start = type(uint256).max; - Bounds memory parentBounds = result[conditions[i].parent]; - if (parentBounds.start == type(uint256).max) { - parentBounds.start = i; - } - parentBounds.end = i + 1; - parentBounds.length = parentBounds.end - parentBounds.start; + // first item is the root + for (uint256 i = 1; i < count; ) { + result[i].start = type(uint256).max; + Bounds memory parentBounds = result[conditions[i].parent]; + if (parentBounds.start == type(uint256).max) { + parentBounds.start = i; + } + parentBounds.end = i + 1; + parentBounds.length = parentBounds.end - parentBounds.start; + unchecked { + ++i; } } } @@ -56,11 +57,13 @@ library Topology { return false; } else { uint256 length = node.children.length; - unchecked { - for (uint256 i; i < length; ++i) { - if (!isInline(node.children[i])) { - return false; - } + + for (uint256 i; i < length; ) { + if (!isInline(node.children[i])) { + return false; + } + unchecked { + ++i; } } return true; @@ -70,24 +73,26 @@ library Topology { function typeTree( Condition memory condition ) internal pure returns (TypeTree memory result) { - unchecked { - if ( - condition.operator >= Operator.And && - condition.operator <= Operator.Nor - ) { - assert(condition.children.length > 0); - return typeTree(condition.children[0]); - } + if ( + condition.operator >= Operator.And && + condition.operator <= Operator.Nor + ) { + assert(condition.children.length > 0); + return typeTree(condition.children[0]); + } - result.paramType = condition.paramType; - if (condition.children.length > 0) { - uint256 length = condition.paramType == ParameterType.Array - ? 1 - : condition.children.length; - result.children = new TypeTree[](length); + result.paramType = condition.paramType; + if (condition.children.length > 0) { + uint256 length = condition.paramType == ParameterType.Array + ? 1 + : condition.children.length; + result.children = new TypeTree[](length); + + for (uint256 i; i < length; ) { + result.children[i] = typeTree(condition.children[i]); - for (uint256 i; i < length; ++i) { - result.children[i] = typeTree(condition.children[i]); + unchecked { + ++i; } } } @@ -114,8 +119,11 @@ library Topology { ? bounds[index].start + 1 : bounds[index].end; result.children = new TypeTree[](end - start); - for (uint256 i = start; i < end; ++i) { + for (uint256 i = start; i < end; ) { result.children[i - start] = typeTree(conditions, i, bounds); + unchecked { + ++i; + } } } } diff --git a/packages/evm/contracts/WriteOnce.sol b/packages/evm/contracts/WriteOnce.sol index 9e865efa6..b078237a7 100644 --- a/packages/evm/contracts/WriteOnce.sol +++ b/packages/evm/contracts/WriteOnce.sol @@ -54,22 +54,20 @@ library WriteOnce { function load( address pointer ) internal view returns (bytes memory runtimeBytecode) { - unchecked { - uint256 rawSize; - assembly { - rawSize := extcodesize(pointer) - } - assert(rawSize > 1); + uint256 rawSize; + assembly { + rawSize := extcodesize(pointer) + } + assert(rawSize > 1); - // jump over the prepended 00 - uint256 offset = 1; - // don't count with the 00 - uint256 size = rawSize - 1; + // jump over the prepended 00 + uint256 offset = 1; + // don't count with the 00 + uint256 size = rawSize - 1; - runtimeBytecode = new bytes(size); - assembly { - extcodecopy(pointer, add(runtimeBytecode, 32), offset, size) - } + runtimeBytecode = new bytes(size); + assembly { + extcodecopy(pointer, add(runtimeBytecode, 32), offset, size) } } diff --git a/packages/evm/contracts/packers/BufferPacker.sol b/packages/evm/contracts/packers/BufferPacker.sol index 26f20ed62..850728ebe 100644 --- a/packages/evm/contracts/packers/BufferPacker.sol +++ b/packages/evm/contracts/packers/BufferPacker.sol @@ -120,32 +120,33 @@ library BufferPacker { result = new ConditionFlat[](count); compValues = new bytes32[](count); - unchecked { - bytes32 word; - uint256 offset = 32; - uint256 compValueOffset = 32 + count * BYTES_PER_CONDITION; + bytes32 word; + uint256 offset = 32; + uint256 compValueOffset = 32 + count * BYTES_PER_CONDITION; - for (uint256 i; i < count; ++i) { + for (uint256 i; i < count; ) { + assembly { + word := mload(add(buffer, offset)) + } + offset += BYTES_PER_CONDITION; + + uint16 bits = uint16(bytes2(word)); + ConditionFlat memory condition = result[i]; + condition.parent = uint8((bits & MASK_PARENT) >> OFFSET_PARENT); + condition.paramType = ParameterType( + (bits & MASK_PARAM_TYPE) >> OFFSET_PARAM_TYPE + ); + condition.operator = Operator(bits & MASK_OPERATOR); + + if (condition.operator >= Operator.EqualTo) { assembly { - word := mload(add(buffer, offset)) - } - offset += BYTES_PER_CONDITION; - - uint16 bits = uint16(bytes2(word)); - ConditionFlat memory condition = result[i]; - condition.parent = uint8((bits & MASK_PARENT) >> OFFSET_PARENT); - condition.paramType = ParameterType( - (bits & MASK_PARAM_TYPE) >> OFFSET_PARAM_TYPE - ); - condition.operator = Operator(bits & MASK_OPERATOR); - - if (condition.operator >= Operator.EqualTo) { - assembly { - word := mload(add(buffer, compValueOffset)) - } - compValueOffset += 32; - compValues[i] = word; + word := mload(add(buffer, compValueOffset)) } + compValueOffset += 32; + compValues[i] = word; + } + unchecked { + ++i; } } } diff --git a/packages/evm/contracts/packers/Packer.sol b/packages/evm/contracts/packers/Packer.sol index 28ffe8818..a364ce11a 100644 --- a/packages/evm/contracts/packers/Packer.sol +++ b/packages/evm/contracts/packers/Packer.sol @@ -78,20 +78,19 @@ library Packer { return false; } else { uint256 length = conditions.length; - unchecked { - for (uint256 j = index + 1; j < length; ++j) { - uint8 parent = conditions[j].parent; - if (parent < index) { - continue; - } - if (parent > index) { - break; - } + for (uint256 j = index + 1; j < length; ++j) { + uint8 parent = conditions[j].parent; + if (parent < index) { + continue; + } + + if (parent > index) { + break; + } - if (!_isInline(conditions, j)) { - return false; - } + if (!_isInline(conditions, j)) { + return false; } } return true; From e340ae998f0932422746c07228a1d79641125753 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Fri, 5 May 2023 10:42:10 +0800 Subject: [PATCH 11/13] Test unexistent allowance --- packages/evm/test/Allowance.spec.ts | 33 +++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/evm/test/Allowance.spec.ts b/packages/evm/test/Allowance.spec.ts index 5fe50f0ec..80e131f8a 100644 --- a/packages/evm/test/Allowance.spec.ts +++ b/packages/evm/test/Allowance.spec.ts @@ -21,6 +21,39 @@ import { } from "./operators/setup"; describe("Allowance", async () => { + it.only("Unexistent allowance produces error", async () => { + const { roles, scopeFunction, invoke } = await loadFixture( + setupTwoParamsStatic + ); + + const allowanceKey = + "0x123000000000000000000000000000000000000000000000000000000000000f"; + + await scopeFunction([ + { + parent: 0, + paramType: ParameterType.AbiEncoded, + operator: Operator.Matches, + compValue: "0x", + }, + { + parent: 0, + paramType: ParameterType.Static, + operator: Operator.WithinAllowance, + compValue: allowanceKey, + }, + { + parent: 0, + paramType: ParameterType.Static, + operator: Operator.Pass, + compValue: "0x", + }, + ]); + + await expect(invoke(100, 100)) + .to.be.revertedWithCustomError(roles, "ConditionViolation") + .withArgs(PermissionCheckerStatus.AllowanceExceeded, allowanceKey); + }); it("consumption in truthy And branch bleeds to other branches", async () => { const { roles, scopeFunction, invoke, owner } = await loadFixture( setupTwoParamsStatic From 9c78b3ba539181ac514c27f370f72e2171ba62d9 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Mon, 8 May 2023 11:22:42 +0200 Subject: [PATCH 12/13] Remove only clause --- packages/evm/test/Allowance.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/evm/test/Allowance.spec.ts b/packages/evm/test/Allowance.spec.ts index 80e131f8a..b5eb9c05b 100644 --- a/packages/evm/test/Allowance.spec.ts +++ b/packages/evm/test/Allowance.spec.ts @@ -21,7 +21,7 @@ import { } from "./operators/setup"; describe("Allowance", async () => { - it.only("Unexistent allowance produces error", async () => { + it("Unexistent allowance produces error", async () => { const { roles, scopeFunction, invoke } = await loadFixture( setupTwoParamsStatic ); From 23b782f781bbda750995934f6fdc01f242faadba Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Mon, 8 May 2023 11:29:39 +0200 Subject: [PATCH 13/13] Format --- packages/evm/contracts/Roles.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/evm/contracts/Roles.sol b/packages/evm/contracts/Roles.sol index 8b47c7d5c..1e1ff680e 100644 --- a/packages/evm/contracts/Roles.sol +++ b/packages/evm/contracts/Roles.sol @@ -11,7 +11,7 @@ import "./PermissionLoader.sol"; * on-chain avatar accounts (like Safe). * @author CristΓ³vΓ£o Honorato - * @author Jan-Felix Schwarz - - * @author Auryn Macmillan - + * @author Auryn Macmillan - * @author Nathan Ginnever - */ contract Roles is