Skip to content

Commit

Permalink
perf: replace calldata slices by abi.decode to extract `ERC725X.exe…
Browse files Browse the repository at this point in the history 
…cute` params (#682)

* refactor: use `abi.decode` instead of calldata slices to extract ERC725X params

* test: update tests for internal functions to use new function parameters

* refactor: remove check for `0x...80` offset and add tests for permissions check

* refactor: adjust + re-order checks for main controller

* test: add more tests for permissions against different data offset

* docs: add LSP6 auto-generated docs

* refactor: move back checks for SUPER permissions to initial position
  • Loading branch information
@CJ42
CJ42 authored Aug 23, 2023
1 parent 1e91548 commit 5b21de8
Show file tree
Hide file tree
Showing 12 changed files with 1,136 additions and 332 deletions.
5 changes: 2 additions & 3 deletions contracts/LSP6KeyManager/LSP6Errors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,9 @@ error AddressPermissionArrayIndexValueNotAnAddress(
error NoERC725YDataKeysAllowed(address from);

/**
* @notice The address `from` is not authorised to use the linked account contract to make external calls, because it has Allowed Calls set.
* @notice The address `from` is not authorised to use the linked account contract to make external calls, because it has no Allowed Calls set.
*
* @dev Reverts when the `from` address has no `AllowedCalls` set and cannot interact with any address
* using the linked {target}.
* @dev Reverts when the `from` address has no `AllowedCalls` set and cannot interact with any address using the linked {target}.
*
* @param from The address that has no AllowedCalls.
*/
Expand Down
18 changes: 14 additions & 4 deletions contracts/LSP6KeyManager/LSP6KeyManagerCore.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ abstract contract LSP6KeyManagerCore is
/**
* @inheritdoc ILSP6KeyManager
*
* @custom:events VerifiedCall event when the permissions related to `payload` have been verified successfully.
* @custom:events {PermissionsVerified} event when the permissions related to `payload` have been verified successfully.
*/
function execute(
bytes calldata payload
Expand All @@ -164,7 +164,7 @@ abstract contract LSP6KeyManagerCore is
/**
* @inheritdoc ILSP6KeyManager
*
* @custom:events VerifiedCall event for each permissions related to each `payload` that have been verified successfully.
* @custom:events {PermissionsVerified} event for each permissions related to each `payload` that have been verified successfully.
*/
function executeBatch(
uint256[] calldata values,
Expand Down Expand Up @@ -199,7 +199,7 @@ abstract contract LSP6KeyManagerCore is
/**
* @inheritdoc ILSP6KeyManager
*
* @custom:events {VerifiedCall} event when the permissions related to `payload` have been verified successfully.
* @custom:events {PermissionsVerified} event when the permissions related to `payload` have been verified successfully.
*
* @custom:hint You can use `validityTimestamps == 0` to define an `executeRelayCall` transaction that is indefinitely valid,
* meaning that does not require to start from a specific date/time, or that has an expiration date/time/
Expand Down Expand Up @@ -528,11 +528,21 @@ abstract contract LSP6KeyManagerCore is

// ERC725X.execute(uint256,address,uint256,bytes)
} else if (erc725Function == IERC725X.execute.selector) {
(
uint256 operationType,
address to,
uint256 value,
bytes memory data
) = abi.decode(payload[4:], (uint256, address, uint256, bytes));

LSP6ExecuteModule._verifyCanExecute(
_target,
from,
permissions,
payload
operationType,
to,
value,
data
);
} else if (
erc725Function == ILSP14Ownable2Step.transferOwnership.selector ||
Expand Down
136 changes: 52 additions & 84 deletions contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,32 +54,16 @@ abstract contract LSP6ExecuteModule {
* @param controlledContract the address of the ERC725 contract where the payload is executed and where the permissions are verified.
* @param controller the address who want to run the execute function on the ERC725Account.
* @param permissions the permissions of the controller address.
* @param payload the ABI encoded payload `controlledContract.execute(...)`.
*/
function _verifyCanExecute(
address controlledContract,
address controller,
bytes32 permissions,
bytes calldata payload
uint256 operationType,
address to,
uint256 value,
bytes memory data
) internal view virtual {
// 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);
}

// MUST be one of the ERC725X operation types.
uint256 operationType = uint256(bytes32(payload[4:36]));

address to = address(bytes20(payload[48:68]));

// if to is the KeyManager address revert
if (to == address(this)) {
revert CallingKeyManagerNotAllowed();
Expand All @@ -92,10 +76,10 @@ abstract contract LSP6ExecuteModule {
// Check to restrict controllers with execute permissions to call lsp20 functions
// to avoid setting the reentrancy guard to a non-valid state

// if (payload.length >= 168 && to == address(this)) {
// if (data.length >= 4 && to == address(this)) {
// if (
// bytes4(payload[164:168]) == ILSP20.lsp20VerifyCall.selector ||
// bytes4(payload[164:168]) == ILSP20.lsp20VerifyCallResult.selector
// bytes4(data) == ILSP20.lsp20VerifyCall.selector ||
// bytes4(data) == ILSP20.lsp20VerifyCallResult.selector
// ) {
// revert CallingLSP20FunctionsOnLSP6NotAllowed();
// }
Expand All @@ -108,7 +92,9 @@ abstract contract LSP6ExecuteModule {
controlledContract,
controller,
permissions,
payload
to,
value,
data
);
}

Expand All @@ -119,7 +105,7 @@ abstract contract LSP6ExecuteModule {
) {
// required to check for permission TRANSFERVALUE if we are funding
// the contract on deployment via a payable constructor
bool isFundingContract = uint256(bytes32(payload[68:100])) != 0;
bool isFundingContract = value != 0;

return
_verifyCanDeployContract(
Expand All @@ -138,7 +124,9 @@ abstract contract LSP6ExecuteModule {
controlledContract,
controller,
permissions,
payload
to,
value,
data
);
}

Expand Down Expand Up @@ -169,7 +157,9 @@ abstract contract LSP6ExecuteModule {
address controlledContract,
address controller,
bytes32 permissions,
bytes calldata payload
address to,
uint256 value,
bytes memory data
) internal view virtual {
bool hasSuperStaticCall = permissions.hasPermission(
_PERMISSION_SUPER_STATICCALL
Expand All @@ -180,36 +170,35 @@ abstract contract LSP6ExecuteModule {

_requirePermissions(controller, permissions, _PERMISSION_STATICCALL);

_verifyAllowedCall(controlledContract, controller, payload);
_verifyAllowedCall(
controlledContract,
controller,
OPERATION_3_STATICCALL,
to,
value,
data
);
}

function _verifyCanCall(
address controlledContract,
address controller,
bytes32 permissions,
bytes calldata payload
address to,
uint256 value,
bytes memory data
) internal view virtual {
bool isTransferringValue = uint256(bytes32(payload[68:100])) != 0;
bool isValueTransfer = value != 0;

bool hasSuperTransferValue = permissions.hasPermission(
_PERMISSION_SUPER_TRANSFERVALUE
);

// all the parameters are abi-encoded (padded to 32 bytes words)
//
// 4 (ERC725X.execute selector)
// + 32 (uint256 operationType)
// + 32 (address to/target)
// + 32 (uint256 value)
// + 32 (`data` offset)
// + 32 (`data` length)
// --------------------
// = 164 bytes in total
bool isCallDataPresent = payload.length > 164;
bool isEmptyCall = data.length == 0;

bool hasSuperCall = permissions.hasPermission(_PERMISSION_SUPER_CALL);

if (isTransferringValue && !hasSuperTransferValue) {
if (isValueTransfer && !hasSuperTransferValue) {
_requirePermissions(
controller,
permissions,
Expand All @@ -219,40 +208,41 @@ abstract contract LSP6ExecuteModule {

// CHECK if we are doing an empty call, as the receive() or fallback() function
// of the controlledContract could run some code.
if (!hasSuperCall && !isCallDataPresent && !isTransferringValue) {
if (isEmptyCall && !isValueTransfer && !hasSuperCall) {
_requirePermissions(controller, permissions, _PERMISSION_CALL);
}

if (isCallDataPresent && !hasSuperCall) {
if (!isEmptyCall && !hasSuperCall) {
_requirePermissions(controller, permissions, _PERMISSION_CALL);
}

// Skip if caller has SUPER permissions for external calls, with or without calldata (empty calls)
if (hasSuperCall && !isTransferringValue) return;
if (!isValueTransfer && hasSuperCall) return;

// Skip if caller has SUPER permission for value transfers
if (hasSuperTransferValue && !isCallDataPresent && isTransferringValue)
return;
if (isEmptyCall && isValueTransfer && hasSuperTransferValue) return;

// Skip if both SUPER permissions are present
if (hasSuperCall && hasSuperTransferValue) return;

_verifyAllowedCall(controlledContract, controller, payload);
_verifyAllowedCall(
controlledContract,
controller,
OPERATION_0_CALL,
to,
value,
data
);
}

function _verifyAllowedCall(
address controlledContract,
address controllerAddress,
bytes calldata payload
uint256 operationType,
address to,
uint256 value,
bytes memory data
) internal view virtual {
(
uint256 operationType,
address to,
uint256 value,
bytes4 selector,
bool isEmptyCall
) = _extractExecuteParameters(payload);

// CHECK for ALLOWED CALLS
bytes memory allowedCalls = ERC725Y(controlledContract)
.getAllowedCallsFor(controllerAddress);
Expand All @@ -261,6 +251,11 @@ abstract contract LSP6ExecuteModule {
revert NoCallsAllowed(controllerAddress);
}

bool isEmptyCall = data.length == 0;

// CHECK if there is at least a 4 bytes function selector
bytes4 selector = data.length >= 4 ? bytes4(data) : bytes4(0);

bytes4 requiredCallTypes = _extractCallType(
operationType,
value,
Expand Down Expand Up @@ -334,33 +329,6 @@ abstract contract LSP6ExecuteModule {
}
}

function _extractExecuteParameters(
bytes calldata executeCalldata
) internal pure returns (uint256, address, uint256, bytes4, bool) {
uint256 operationType = uint256(bytes32(executeCalldata[4:36]));

// CHECK that it is a valid address left-padded with `00` on the 12 upper bytes
if (bytes12(executeCalldata[36:48]) != bytes12(0)) {
revert InvalidPayload(executeCalldata);
}
address to = address(bytes20(executeCalldata[48:68]));

uint256 value = uint256(bytes32(executeCalldata[68:100]));

// CHECK if there is at least a 4 bytes function selector
bytes4 selector = executeCalldata.length >= 168
? bytes4(executeCalldata[164:168])
: bytes4(0);

return (
operationType,
to,
value,
selector,
executeCalldata.length == 164
);
}

function _isAllowedAddress(
bytes memory allowedCall,
address to
Expand Down
24 changes: 18 additions & 6 deletions contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,19 @@ contract KeyManagerInternalTester is LSP6KeyManager {

function verifyAllowedCall(
address _sender,
bytes calldata _payload
uint256 operationType,
address to,
uint256 value,
bytes memory data
) public view {
super._verifyAllowedCall(_target, _sender, _payload);
super._verifyAllowedCall(
_target,
_sender,
operationType,
to,
value,
data
);
}

function isCompactBytesArrayOfAllowedCalls(
Expand Down Expand Up @@ -103,9 +113,11 @@ contract KeyManagerInternalTester is LSP6KeyManager {
return _addressPermission.hasPermission(_permissions);
}

function extractExecuteParameters(
bytes calldata executeCalldata
) public pure returns (uint256, address, uint256, bytes4, bool) {
return super._extractExecuteParameters(executeCalldata);
function verifyPermissions(
address from,
uint256 msgValue,
bytes calldata payload
) public view {
super._verifyPermissions(from, msgValue, payload);
}
}
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
Loading

0 comments on commit 5b21de8

Please sign in to comment.