Skip to content

Commit

Permalink
refactor: remove check for 0x...80 offset and add tests for permiss…
Browse files Browse the repository at this point in the history 
…ions check
  • Loading branch information
@CJ42
CJ42 committed Aug 18, 2023
1 parent 25e1385 commit 75681b0
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 49 deletions.
13 changes: 0 additions & 13 deletions contracts/LSP6KeyManager/LSP6KeyManagerCore.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -528,19 +528,6 @@ abstract contract LSP6KeyManagerCore is

// ERC725X.execute(uint256,address,uint256,bytes)
} else if (erc725Function == IERC725X.execute.selector) {
// CHECK the offset of `data` is not pointing to the previous parameters
//
// offsets in calldata for ERC725X.execute(...) parameters (excluding function selector)
//
// - `operationType`: index 0 in calldata
// - `to`: index 32
// - `value`: index 64
// - `data`'s offset location: index 96
// - `data` starts at: index 128 (= 0x0000...0080)
if (bytes32(payload[100:132]) != bytes32(uint256(128))) {
revert InvalidPayload(payload);
}

(
uint256 operationType,
address to,
Expand Down
4 changes: 4 additions & 0 deletions contracts/Mocks/TargetContract.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ contract TargetContract {
}

function setNamePayable(string memory name) public payable {
_name = name;
}

function setNamePayableMinimumValue(string memory name) public payable {
require(msg.value >= 50, "Not enough value provided");
_name = name;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP.behaviour.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import { ERC725YDataKeys, INTERFACE_IDS, OPERATION_TYPES, LSP1_TYPE_IDS } from '
// fixtures
import { callPayload, getLSP5MapAndArrayKeysValue, setupKeyManager } from '../utils/fixtures';
import { LSP6TestContext } from '../utils/context';
import { BigNumber, BytesLike, ContractTransaction, Transaction } from 'ethers';
import { BigNumber, BytesLike, Transaction } from 'ethers';

export type LSP1TestAccounts = {
owner1: SignerWithAddress;
Expand Down
160 changes: 134 additions & 26 deletions tests/LSP6KeyManager/Interactions/PermissionCall.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,18 @@ import { LSP6TestContext } from '../../utils/context';
import { setupKeyManager } from '../../utils/fixtures';

// helpers
import { abiCoder, combineAllowedCalls, LOCAL_PRIVATE_KEYS } from '../../utils/helpers';

export const shouldBehaveLikePermissionCall = (buildContext: () => Promise<LSP6TestContext>) => {
import {
abiCoder,
combineAllowedCalls,
combineCallTypes,
combinePermissions,
LOCAL_PRIVATE_KEYS,
} from '../../utils/helpers';
import { BigNumber } from 'ethers';

export const shouldBehaveLikePermissionCall = (
buildContext: (initialFunding?: BigNumber) => Promise<LSP6TestContext>,
) => {
let context: LSP6TestContext;

describe('when making an empty call via `ERC25X.execute(...)` -> (`data` = `0x`, `value` = 0)', () => {
Expand Down Expand Up @@ -407,29 +416,6 @@ export const shouldBehaveLikePermissionCall = (buildContext: () => Promise<LSP6T
await setupKeyManager(context, permissionKeys, permissionsValues);
});

describe("when the 'offset' of the `data` payload is not `0x00...80`", () => {
it('should revert', async () => {
let payload = context.universalProfile.interface.encodeFunctionData('execute', [
OPERATION_TYPES.CALL,
targetContract.address,
0,
'0xcafecafe',
]);

// edit the `data` offset
payload = payload.replace(
'0000000000000000000000000000000000000000000000000000000000000080',
'0000000000000000000000000000000000000000000000000000000000000040',
);

await expect(
context.keyManager.connect(addressCanMakeCallWithAllowedCalls).execute(payload),
)
.to.be.revertedWithCustomError(context.keyManager, 'InvalidPayload')
.withArgs(payload);
});
});

describe('when interacting via `execute(...)`', () => {
describe('when caller has ALL PERMISSIONS', () => {
it('should pass and change state at the target contract', async () => {
Expand Down Expand Up @@ -1050,5 +1036,127 @@ export const shouldBehaveLikePermissionCall = (buildContext: () => Promise<LSP6T
context.keyManager.connect(context.owner).execute(executePayload),
).to.be.revertedWithCustomError(context.keyManager, 'CallingKeyManagerNotAllowed');
});

describe('when the offset of the `data` payload is not `0x00...80`', () => {
describe('if the offset points backwards to the `value` parameter', () => {
// We add the target in the allowed calls for each of these controller
let controllerCanTransferValue;
let controllerCanTransferValueAndCall;
let controllerCanCall;

let controllerCanOnlySign;

let targetContract;

let executePayload;

before(async () => {
context = await buildContext(ethers.utils.parseEther('50'));

const accounts = await ethers.getSigners();

controllerCanTransferValue = accounts[1];
controllerCanTransferValueAndCall = accounts[2];
controllerCanCall = accounts[3];
controllerCanOnlySign = accounts[4];

targetContract = await new TargetContract__factory(context.accounts[0]).deploy();

const permissionKeys = [
// permissions
ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] +
controllerCanTransferValue.address.substring(2),
ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] +
controllerCanTransferValueAndCall.address.substring(2),
ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] +
controllerCanCall.address.substring(2),
ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] +
controllerCanOnlySign.address.substring(2),
// allowed calls
ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] +
controllerCanTransferValue.address.substring(2),
ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] +
controllerCanTransferValueAndCall.address.substring(2),
ERC725YDataKeys.LSP6['AddressPermissions:AllowedCalls'] +
controllerCanCall.address.substring(2),
];

const allowedCall = combineAllowedCalls(
[combineCallTypes(CALLTYPE.CALL, CALLTYPE.VALUE)],
[targetContract.address],
['0xffffffff'],
['0xffffffff'],
);

const permissionValues = [
// permissions
PERMISSIONS.TRANSFERVALUE,
combinePermissions(PERMISSIONS.TRANSFERVALUE, PERMISSIONS.CALL),
PERMISSIONS.CALL,
PERMISSIONS.SIGN,
// allowed calls,
allowedCall,
allowedCall,
allowedCall,
];

await setupKeyManager(context, permissionKeys, permissionValues);

executePayload = context.universalProfile.interface.encodeFunctionData('execute', [
OPERATION_TYPES.CALL,
targetContract.address,
36,
'0xdeadbeef',
]);

// edit the `data` offset to points to the `value` parameter
executePayload = executePayload.replace(
'0000000000000000000000000000000000000000000000000000000000000080',
'0000000000000000000000000000000000000000000000000000000000000040',
);
});

describe('when caller has permission TRANSFERVALUE only', () => {
it("should revert with 'NotAuthorised' error to 'CALL'", async () => {
await expect(
context.keyManager.connect(controllerCanTransferValue).execute(executePayload),
)
.to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised')
.withArgs(controllerCanTransferValue.address, 'CALL');
});
});

describe('when caller has permission CALL only', () => {
it("should revert with 'NotAuthorised' error to 'TRANSFERVALUE'", async () => {
await expect(
context.keyManager.connect(controllerCanCall).execute(executePayload, { value: 100 }),
)
.to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised')
.withArgs(controllerCanCall.address, 'TRANSFERVALUE');
});
});

describe('when caller does not have neither CALL nor TRANSFERVALUE permissions', () => {
it("should revert with 'NotAuthorised' error to 'TRANSFERVALUE' (as value transfer is the first thing being checked", async () => {
await expect(context.keyManager.connect(controllerCanOnlySign).execute(executePayload))
.to.be.revertedWithCustomError(context.keyManager, 'NotAuthorised')
.withArgs(controllerCanOnlySign.address, 'TRANSFERVALUE');
});
});

describe('when caller has both permissions CALL + TRANSFERVALUE', () => {
it('should pass and allow to call the contract (but fallback to default handler because of unrecognised selector)', async () => {
// Here the controller has the permissions to do the external call
// but the default error handler is triggered because there is no function selector `0x40deadbe'
// in the contract and it is not recognised
await expect(
context.keyManager
.connect(controllerCanTransferValueAndCall)
.callStatic.execute(executePayload),
).to.be.revertedWith('ERC725X: Unknown Error');
});
});
});
});
});
};
12 changes: 6 additions & 6 deletions tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,11 +336,11 @@ export const shouldBehaveLikeExecuteRelayCall = (
it('should revert', async () => {
const nameToSet = 'Alice';
const targetContractPayload = targetContract.interface.encodeFunctionData(
'setNamePayable',
'setNamePayableMinimumValue',
[nameToSet],
);

const requiredValueForExecution = 51; // specified in `setNamePayable(..)`
const requiredValueForExecution = 51; // specified in `setNamePayableMinimumValue(..)`

const latestNonce = await context.keyManager.callStatic.getNonce(signer.address, 0);

Expand Down Expand Up @@ -413,11 +413,11 @@ export const shouldBehaveLikeExecuteRelayCall = (
it('should pass if signer has the target contract address in its list of allowed calls', async () => {
const nameToSet = 'Alice';
const targetContractPayload = targetContract.interface.encodeFunctionData(
'setNamePayable',
'setNamePayableMinimumValue',
[nameToSet],
);

const requiredValueForExecution = 51; // specified in `setNamePayable(..)`
const requiredValueForExecution = 51; // specified in `setNamePayableMinimumValue(..)`

const latestNonce = await context.keyManager.callStatic.getNonce(signer.address, 0);

Expand Down Expand Up @@ -481,11 +481,11 @@ export const shouldBehaveLikeExecuteRelayCall = (
it("should revert with 'NotAllowedCall' error if signer does not have any listed under its allowed calls", async () => {
const nameToSet = 'Alice';
const targetContractPayload = targetContract.interface.encodeFunctionData(
'setNamePayable',
'setNamePayableMinimumValue',
[nameToSet],
);

const requiredValueForExecution = 51; // specified in `setNamePayable(..)`
const requiredValueForExecution = 51; // specified in `setNamePayableMinimumValue(..)`

const latestNonce = await context.keyManager.callStatic.getNonce(
signerNoAllowedCalls.address,
Expand Down
4 changes: 1 addition & 3 deletions tests/LSP6KeyManager/internals/AllowedCalls.internal.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { expect } from 'chai';
import { ethers } from 'hardhat';
import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers';

import { TargetContract, TargetContract__factory, UniversalProfile__factory } from '../../../types';
import { TargetContract, TargetContract__factory } from '../../../types';

// constants
import {
Expand Down Expand Up @@ -714,8 +714,6 @@ export const testAllowedCallsInternals = (

describe('should revert with `NoCallsAllowed` error', () => {
it(`when AllowedCalls contain noBytes -> 0x`, async () => {
const params = Object.values(executeParams);

await expect(
context.keyManagerInternalTester.verifyAllowedCall(
controllers[0].account.address,
Expand Down

0 comments on commit 75681b0

Please sign in to comment.