Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: add callee params to lsp20 & EXECUTE_RELAY_CALL permission & mapping reentrancyStatus #729

Merged
merged 25 commits into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
0825ae3
feat!: add callee params to lsp20
skimaharvey Sep 26, 2023
7b27c64
chore: update lsp6 to with new lsp20 interface
skimaharvey Sep 26, 2023
79b7b06
test: update tests to work with new lsp20 interface
skimaharvey Sep 26, 2023
b0552a4
refactor: update constants.ts file
skimaharvey Sep 26, 2023
f5bfd09
test: fix failing tests
skimaharvey Sep 26, 2023
5b52316
test: fix foundry tests
skimaharvey Sep 26, 2023
62e2679
docs: generate lsp6 new abi docs
skimaharvey Sep 26, 2023
085dbad
docs: update lsp6 interfaceID
skimaharvey Sep 26, 2023
5889edf
feat!: add EXECUTE_RELAY_CALL permission
skimaharvey Sep 26, 2023
234fb6e
refactor: move `EXECUTE_RELAY_CALL` permission as last bit in permiss…
CJ42 Sep 27, 2023
a0545ab
test: fix lsp6 tests
skimaharvey Sep 27, 2023
4a0696a
feat!: add LSP6ExecuteRelayCallModule to LSP6
skimaharvey Sep 30, 2023
e72566b
test: add EXECUTE_RELAY_CALL test
skimaharvey Sep 30, 2023
ca9c9dd
test: modify reentrancy tests with EXECUTE_RELAY_CALL permission
skimaharvey Sep 30, 2023
69556c3
Merge branch 'develop' of https://github.com/lukso-network/lsp-smart-…
skimaharvey Sep 30, 2023
8736873
refactor: use uint8 for reentrancyStatus
skimaharvey Sep 30, 2023
550b608
docs: build lsp6 docs
skimaharvey Sep 30, 2023
09c283e
test: fix reentrancy test
skimaharvey Oct 1, 2023
32ae896
refactor: use mapping for reentrancyStatus
skimaharvey Oct 1, 2023
8cb66d4
refactor: rename callee to targetContract
skimaharvey Oct 1, 2023
91fcc47
docs: update docs with targetContract
skimaharvey Oct 1, 2023
685872c
refactor: use uint256 for `reentrancyStatus`
skimaharvey Oct 2, 2023
7850888
Merge branch 'develop' of github.com:lukso-network/lsp-smart-contract…
skimaharvey Oct 2, 2023
7ae6051
refactor: use bool for reentrancyStatus
skimaharvey Oct 2, 2023
8e9963c
reactor: remove cast to bytes1 in _verifyCall
skimaharvey Oct 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const INTERFACE_IDS = {
ERC725Y: '0x629aa694',
LSP0ERC725Account: '0x24871b3d',
LSP1UniversalReceiver: '0x6bb56a14',
LSP6KeyManager: '0x66918867',
LSP6KeyManager: '0xe7424397',
LSP7DigitalAsset: '0x05519512',
LSP8IdentifiableDigitalAsset: '0x1ae9ba1f',
LSP9Vault: '0x28af17e6',
Expand All @@ -35,7 +35,7 @@ export const INTERFACE_IDS = {
LSP17Extendable: '0xa918fa6b',
LSP17Extension: '0xcee78b40',
LSP20CallVerification: '0x1a0eb6a5',
LSP20CallVerifier: '0x480c0ec2',
LSP20CallVerifier: '0xc9dfc532',
LSP25ExecuteRelayCall: '0x5ac79908',
};

Expand All @@ -61,9 +61,9 @@ export const ERC1271_VALUES = {
export const LSP20_MAGIC_VALUES = {
VERIFY_CALL: {
// bytes3(keccak256("lsp20VerifyCall(address,uint256,bytes)")) + "0x01"
skimaharvey marked this conversation as resolved.
Show resolved Hide resolved
WITH_POST_VERIFICATION: '0x9bf04b01',
WITH_POST_VERIFICATION: '0x1a238001',
// bytes3(keccak256("lsp20VerifyCall(address,uint256,bytes)")) + "0x00"
skimaharvey marked this conversation as resolved.
Show resolved Hide resolved
NO_POST_VERIFICATION: '0x9bf04b00',
NO_POST_VERIFICATION: '0x1a238000',
},
// bytes4(keccak256("lsp20VerifyCallResult(bytes32,bytes)"))
VERIFY_CALL_RESULT: '0xd3fc45d3',
Expand Down
2 changes: 2 additions & 0 deletions contracts/LSP20CallVerification/ILSP20CallVerifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ interface ILSP20CallVerifier {
* the function is allowed, concatened with a byte that determines if the lsp20VerifyCallResult function should
* be called after the original function call. The byte that invoke the lsp20VerifyCallResult function is strictly `0x01`.
*
* @param callee The address of the contract that implements the LSP20 interface
skimaharvey marked this conversation as resolved.
Show resolved Hide resolved
* @param caller The address who called the function on the msg.sender
* @param value The value sent by the caller to the function called on the msg.sender
* @param receivedCalldata The receivedCalldata sent by the caller to the msg.sender
*/
function lsp20VerifyCall(
address callee,
skimaharvey marked this conversation as resolved.
Show resolved Hide resolved
address caller,
uint256 value,
bytes memory receivedCalldata
Expand Down
10 changes: 9 additions & 1 deletion contracts/LSP20CallVerification/LSP20CallVerification.sol
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ abstract contract LSP20CallVerification {
(bool success, bytes memory returnedData) = logicVerifier.call(
abi.encodeWithSelector(
ILSP20.lsp20VerifyCall.selector,
address(this),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to make the LSP20 modules reusable, I would suggest to parameterize in _verifyCall all the lsp20 functions parameter, because not in all scenarios the address(this) will be the target or msg.sender is the one to verify permissions for.

But probably can be done in later PRs, just a note

msg.sender,
msg.value,
msg.data
Expand Down Expand Up @@ -56,7 +57,14 @@ abstract contract LSP20CallVerification {
(bool success, bytes memory returnedData) = logicVerifier.call(
abi.encodeWithSelector(
ILSP20.lsp20VerifyCallResult.selector,
keccak256(abi.encodePacked(msg.sender, msg.value, msg.data)),
keccak256(
abi.encodePacked(
address(this),
msg.sender,
msg.value,
msg.data
)
),
callResult
)
);
Expand Down
8 changes: 4 additions & 4 deletions contracts/LSP20CallVerification/LSP20Constants.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ pragma solidity ^0.8.4;
// bytes4(keccak256("LSP20CallVerification"))
bytes4 constant _INTERFACEID_LSP20_CALL_VERIFICATION = 0x1a0eb6a5;

// `lsp20VerifyCall(address,uint256,bytes)` selector XOR `lsp20VerifyCallResult(bytes32,bytes)` selector
bytes4 constant _INTERFACEID_LSP20_CALL_VERIFIER = 0x480c0ec2;
// `lsp20VerifyCall(address,address,uint256,bytes)` selector XOR `lsp20VerifyCallResult(bytes32,bytes)` selector
bytes4 constant _INTERFACEID_LSP20_CALL_VERIFIER = 0xc9dfc532;

// bytes4(bytes.concat(bytes3(ILSP20.lsp20VerifyCall.selector), hex"01"))
bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION = 0x9bf04b01;
bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION = 0x1a238001;

// bytes4(bytes.concat(bytes3(ILSP20.lsp20VerifyCall.selector), hex"00"))
bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION = 0x9bf04b00;
bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION = 0x1a238000;

// bytes4(ILSP20.lsp20VerifyCallResult.selector)
bytes4 constant _LSP20_VERIFY_CALL_RESULT_MAGIC_VALUE = 0xd3fc45d3;
2 changes: 1 addition & 1 deletion contracts/LSP6KeyManager/LSP6Constants.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
pragma solidity ^0.8.4;

// --- ERC165 interface ids
bytes4 constant _INTERFACEID_LSP6 = 0x66918867;
bytes4 constant _INTERFACEID_LSP6 = 0xe7424397;

// --- ERC725Y Data Keys

Expand Down
26 changes: 14 additions & 12 deletions contracts/LSP6KeyManager/LSP6KeyManagerCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -313,10 +313,11 @@ abstract contract LSP6KeyManagerCore is
* on the {`target`} contract (while sending `msgValue` alongside the call).
*
* If the permissions have been verified successfully and `caller` is authorized, one of the following two LSP20 magic value will be returned:
* - `0x9bf04b00`: LSP20 magic value **without** post verification (last byte is `0x00`).
* - `0x9bf04b01`: LSP20 magic value **with** post-verification (last byte is `0x01`).
* - `0x1a238000`: LSP20 magic value **without** post verification (last byte is `0x00`).
* - `0x1a238001`: LSP20 magic value **with** post-verification (last byte is `0x01`).
*/
function lsp20VerifyCall(
address callee,
address caller,
uint256 msgValue,
bytes calldata data
Expand All @@ -330,10 +331,10 @@ abstract contract LSP6KeyManagerCore is
}

// If target is invoking the verification, emit the event and change the reentrancy guard
if (msg.sender == _target) {
if (msg.sender == callee) {
bool isReentrantCall = _nonReentrantBefore(isSetData, caller);

_verifyPermissions(caller, msgValue, data);
_verifyPermissions(callee, caller, msgValue, data);
emit PermissionsVerified(caller, msgValue, bytes4(data));

// if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function
Expand All @@ -350,12 +351,12 @@ abstract contract LSP6KeyManagerCore is
if (isReentrantCall) {
_requirePermissions(
caller,
ERC725Y(_target).getPermissionsFor(caller),
ERC725Y(callee).getPermissionsFor(caller),
_PERMISSION_REENTRANCY
);
}

_verifyPermissions(caller, msgValue, data);
_verifyPermissions(callee, caller, msgValue, data);

// if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function
return
Expand Down Expand Up @@ -398,7 +399,7 @@ abstract contract LSP6KeyManagerCore is

bool isReentrantCall = _nonReentrantBefore(isSetData, msg.sender);

_verifyPermissions(msg.sender, msgValue, payload);
_verifyPermissions(_target, msg.sender, msgValue, payload);
emit PermissionsVerified(msg.sender, msgValue, bytes4(payload));

bytes memory result = _executePayload(msgValue, payload);
Expand Down Expand Up @@ -463,7 +464,7 @@ abstract contract LSP6KeyManagerCore is

bool isReentrantCall = _nonReentrantBefore(isSetData, signer);

_verifyPermissions(signer, msgValue, payload);
_verifyPermissions(_target, signer, msgValue, payload);
emit PermissionsVerified(signer, msgValue, bytes4(payload));

bytes memory result = _executePayload(msgValue, payload);
Expand Down Expand Up @@ -503,11 +504,12 @@ abstract contract LSP6KeyManagerCore is
* @param payload The abi-encoded function call to execute on the {target} contract.
*/
function _verifyPermissions(
address callee,
address from,
uint256 msgValue,
bytes calldata payload
) internal view virtual {
bytes32 permissions = ERC725Y(_target).getPermissionsFor(from);
bytes32 permissions = ERC725Y(callee).getPermissionsFor(from);
if (permissions == bytes32(0)) revert NoPermissionsSet(from);

bytes4 erc725Function = bytes4(payload);
Expand All @@ -521,7 +523,7 @@ abstract contract LSP6KeyManagerCore is
);

LSP6SetDataModule._verifyCanSetData(
_target,
callee,
from,
permissions,
inputKey,
Expand All @@ -535,7 +537,7 @@ abstract contract LSP6KeyManagerCore is
.decode(payload[4:], (bytes32[], bytes[]));

LSP6SetDataModule._verifyCanSetData(
_target,
callee,
from,
permissions,
inputKeys,
Expand All @@ -552,7 +554,7 @@ abstract contract LSP6KeyManagerCore is
) = abi.decode(payload[4:], (uint256, address, uint256, bytes));

LSP6ExecuteModule._verifyCanExecute(
_target,
callee,
from,
permissions,
operationType,
Expand Down
2 changes: 1 addition & 1 deletion contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ contract KeyManagerInternalTester is LSP6KeyManager {
uint256 msgValue,
bytes calldata payload
) public {
super._verifyPermissions(from, msgValue, payload);
super._verifyPermissions(_target, from, msgValue, payload);

// This event is emitted just for a sake of not marking this function as `view`,
// as Hardhat has a bug that does not catch error that occured from failed `abi.decode`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@ contract FirstCallReturnExpandedInvalidValue {
address public target;

function lsp20VerifyCall(
address callee,
address caller,
uint256 value,
bytes memory data
) external returns (bytes32) {
emit CallVerified();

return keccak256(abi.encode(caller, value, data));
return keccak256(abi.encode(callee, caller, value, data));
}

function acceptOwnership(address newTarget) external {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ contract FirstCallReturnInvalidMagicValue {
address public target;

function lsp20VerifyCall(
address,
address,
uint256,
bytes memory
Expand Down
4 changes: 2 additions & 2 deletions docs/_interface_ids_table.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
| **ERC725Y** | `0x629aa694` | General Data key-value store. |
| **LSP0ERC725Account** | `0x24871b3d` | Interface of the [LSP-0-ERC725Account] standard, an account based smart contract that represents an identity on-chain. |
| **LSP1UniversalReceiver** | `0x6bb56a14` | Interface of the LSP1 - Universal Receiver standard, an entry function for a contract to receive arbitrary information. |
| **LSP6KeyManager** | `0x66918867` | Interface of the LSP6 - Key Manager standard, a contract acting as a controller of an ERC725 Account using predfined permissions. |
| **LSP6KeyManager** | `0xe7424397` | Interface of the LSP6 - Key Manager standard, a contract acting as a controller of an ERC725 Account using predfined permissions. |
| **LSP7DigitalAsset** | `0x05519512` | Interface of the LSP7 - Digital Asset standard, a fungible digital asset. |
| **LSP8IdentifiableDigitalAsset** | `0x1ae9ba1f` | Interface of the LSP8 - Identifiable Digital Asset standard, a non-fungible digital asset. |
| **LSP9Vault** | `0x28af17e6` | Interface of LSP9 - Vault standard, a blockchain vault that can hold assets and interact with other smart contracts. |
Expand All @@ -15,5 +15,5 @@
| **LSP17Extendable** | `0xa918fa6b` | Module to add more functionalities to a contract using extensions. |
| **LSP17Extension** | `0xcee78b40` | Module to create a contract that can act as an extension. |
| **LSP20CallVerification** | `0x1a0eb6a5` | Implementation of a contract calling the verification functions according to LSP20 - Call Verification standard. |
| **LSP20CallVerifier** | `0x480c0ec2` | Interface for the LSP20 Call Verification standard, a set of functions intended to perform verifications on behalf of another contract. |
| **LSP20CallVerifier** | `0xc9dfc532` | Interface for the LSP20 Call Verification standard, a set of functions intended to perform verifications on behalf of another contract. |
| **LSP25ExecuteRelayCall** | `0x5ac79908` | |
22 changes: 13 additions & 9 deletions docs/contracts/LSP6KeyManager/LSP6KeyManager.md
Original file line number Diff line number Diff line change
Expand Up @@ -332,22 +332,23 @@ Checks if a signature was signed by a controller that has the permission `SIGN`.

- Specification details: [**LSP-6-KeyManager**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-6-KeyManager.md#lsp20verifycall)
- Solidity implementation: [`LSP6KeyManager.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP6KeyManager/LSP6KeyManager.sol)
- Function signature: `lsp20VerifyCall(address,uint256,bytes)`
- Function selector: `0x9bf04b11`
- Function signature: `lsp20VerifyCall(address,address,uint256,bytes)`
- Function selector: `0x1a2380e1`

:::

:::tip Hint

This function can call by any other address than the {`target`}. This allows to verify permissions in a _"read-only"_ manner. Anyone can call this function to verify if the `caller` has the right permissions to perform the abi-encoded function call `data` on the {`target`} contract (while sending `msgValue` alongside the call). If the permissions have been verified successfully and `caller` is authorized, one of the following two LSP20 magic value will be returned:

- `0x9bf04b00`: LSP20 magic value **without** post verification (last byte is `0x00`).
- `0x9bf04b01`: LSP20 magic value **with** post-verification (last byte is `0x01`).
- `0x1a238000`: LSP20 magic value **without** post verification (last byte is `0x00`).
- `0x1a238001`: LSP20 magic value **with** post-verification (last byte is `0x01`).

:::

```solidity
function lsp20VerifyCall(
address callee,
address caller,
uint256 msgValue,
bytes data
Expand All @@ -356,11 +357,12 @@ function lsp20VerifyCall(

#### Parameters

| Name | Type | Description |
| ---------- | :-------: | ----------------------------------------------------- |
| `caller` | `address` | The address who called the function on the msg.sender |
| `msgValue` | `uint256` | - |
| `data` | `bytes` | - |
| Name | Type | Description |
| ---------- | :-------: | --------------------------------------------------------------- |
| `callee` | `address` | The address of the contract that implements the LSP20 interface |
| `caller` | `address` | The address who called the function on the msg.sender |
| `msgValue` | `uint256` | - |
| `data` | `bytes` | - |

#### Returns

Expand Down Expand Up @@ -1150,6 +1152,7 @@ _Execute the `payload` passed to `execute(...)` or `executeRelayCall(...)`_

```solidity
function _verifyPermissions(
address callee,
address from,
uint256 msgValue,
bytes payload
Expand All @@ -1162,6 +1165,7 @@ Verify if the `from` address is allowed to execute the `payload` on the [`target

| Name | Type | Description |
| ---------- | :-------: | ------------------------------------------------------------------- |
| `callee` | `address` | - |
| `from` | `address` | Either the caller of {execute} or the signer of {executeRelayCall}. |
| `msgValue` | `uint256` | - |
| `payload` | `bytes` | The abi-encoded function call to execute on the {target} contract. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ export const shouldBehaveLikePermissionCall = (
it('Should revert when caller calls the KeyManager through execute', async () => {
const lsp20VerifyCallPayload = context.keyManager.interface.encodeFunctionData(
'lsp20VerifyCall',
[context.accounts[2].address, 0, '0xaabbccdd'], // random arguments
[context.keyManager.address, context.accounts[2].address, 0, '0xaabbccdd'], // random arguments
);

await expect(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export const testSecurityScenarios = (buildContext: () => Promise<LSP6TestContex
it('Should revert when caller calls the KeyManager through `ERC725X.execute`', async () => {
const lsp20VerifyCallPayload = context.keyManager.interface.encodeFunctionData(
'lsp20VerifyCall',
[context.accounts[2].address, 0, '0xaabbccdd'], // random arguments
[context.keyManager.address, context.accounts[2].address, 0, '0xaabbccdd'], // random arguments
);

await expect(
Expand Down Expand Up @@ -290,6 +290,7 @@ export const testSecurityScenarios = (buildContext: () => Promise<LSP6TestContex
]);

const tx = await context.keyManager.lsp20VerifyCall(
context.universalProfile.address,
context.owner.address,
0,
emptyCallPayload,
Expand Down
2 changes: 1 addition & 1 deletion tests/LSP6KeyManager/Interactions/PermissionCall.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1023,7 +1023,7 @@ export const shouldBehaveLikePermissionCall = (
it('Should revert when caller calls the KeyManager through execute', async () => {
const lsp20VerifyCallPayload = context.keyManager.interface.encodeFunctionData(
'lsp20VerifyCall',
[context.accounts[2].address, 0, '0xaabbccdd'], // random arguments
[context.keyManager.address, context.accounts[2].address, 0, '0xaabbccdd'], // random arguments
);

const executePayload = context.universalProfile.interface.encodeFunctionData('execute', [
Expand Down
14 changes: 12 additions & 2 deletions tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,12 @@ contract LSP6SetDataTest is Test {
keyManager.execute(callData);

// CHECK the LSP20 verification function reverts as well
keyManager.lsp20VerifyCall(malicious, 0, functionArgs);
keyManager.lsp20VerifyCall(
address(universalProfile),
malicious,
0,
functionArgs
);

// CHECK it reverts when calling directly the Universal Profile
universalProfile.setData(dataKey, dataValue);
Expand Down Expand Up @@ -222,7 +227,12 @@ contract LSP6SetDataTest is Test {
keyManager.execute(callData);

// CHECK the LSP20 verification function reverts as well
keyManager.lsp20VerifyCall(malicious, 0, functionArgs);
keyManager.lsp20VerifyCall(
address(universalProfile),
malicious,
0,
functionArgs
);

// CHECK it reverts when calling directly the Universal Profile
universalProfile.setData(dataKey, dataValue);
Expand Down
Loading