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

WIP:: Custom method selector SVM (ABI SVM) #174

Merged
merged 40 commits into from
Feb 16, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
757e0df
:sparkles: ABI SVM
Nov 24, 2023
82050a0
happy path test
Nov 24, 2023
cabf1e0
:white_check_mark: bundler test happy path
Nov 24, 2023
871c657
:white_check_mark: regular test case for wrong selector
Nov 24, 2023
cdaa8ea
:art: linting
Nov 24, 2023
06e34f0
minor change natspec
Nov 28, 2023
9e91cd2
Interface added
Dec 16, 2023
5e2a82c
:zap: minor optimization re working with calldata
Dec 16, 2023
9f29997
:zap: basis for further optimizations
Dec 16, 2023
ee5a00f
:art: linting
Dec 20, 2023
a4a56fc
Merge branch 'develop' into feat/SMA-363-ABI-SVM
Dec 28, 2023
d5707ca
:white_checkmark: add batch test for ABI SVM
Dec 28, 2023
0282abb
:zap: optimized permission decode
Dec 28, 2023
009cb62
:art: clean
Dec 28, 2023
3ad1f1d
:zap: changes upon review
Dec 28, 2023
b709fc3
:zap: optimizations + natspec
Dec 28, 2023
bd875c5
switch to packed encoding of the permission
Jan 9, 2024
7079ddb
apply changes to tests, remove interface
Jan 9, 2024
57cdcaf
update lockfile
livingrockrises Jan 10, 2024
ba2adf9
refactor lint
livingrockrises Jan 10, 2024
8027bc8
:white_check_mark: more tests
Jan 10, 2024
dc0b513
add test
Jan 10, 2024
25a5300
:white_check_mark: add tests
Jan 10, 2024
bafd70c
intendation fix
Jan 10, 2024
76110d9
Merge branch 'feat/SMA-363-ABI-SVM' of https://github.com/bcnmy/scw-c…
Jan 10, 2024
af794e4
pull latest changes
Jan 10, 2024
d1916c1
fix mock protocol to pass gh test
Jan 10, 2024
6f4a410
check for non-existent condition upon self-review
Jan 12, 2024
189ae66
clean and comments
Jan 12, 2024
724d6a2
natspec
Jan 12, 2024
3784b38
:art: linting
Jan 12, 2024
78add8c
empty rules case
Jan 15, 2024
836b818
:art: constants and comments
Jan 15, 2024
2118575
incorrect rules list length test
Jan 15, 2024
5df0325
wrong rules list length test case
Jan 15, 2024
ef0053c
natspec fix
Feb 6, 2024
6de8982
bytes32=>uint160=>address conversion for destContract
Feb 8, 2024
dddc887
Merge pull request #197 from bcnmy/feat/SMA-378-379-ABI-SVM-Audit-Rem…
filmakarov Feb 14, 2024
0d4251f
Merge branch 'develop' into feat/SMA-363-ABI-SVM
Feb 14, 2024
73dcb23
add Kawach report
Feb 16, 2024
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
3 changes: 2 additions & 1 deletion .solhintignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ node_modules
artifacts
contracts/smart-account/test
contracts/smart-account/libs
contracts/smart-account/modules/AccountRecoveryModule.sol
contracts/smart-account/modules/AccountRecoveryModule.sol
contracts/smart-account/modules/SessionValidationModules/ABISessionValidationModule.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import "../../interfaces/modules/ISessionValidationModule.sol";

/**
* @title ABI Session Validation Module for Biconomy Smart Accounts.
* @dev Validates userOps for any contract / method / params.
* The _sessionKeyData layout:
filmakarov marked this conversation as resolved.
Show resolved Hide resolved
* Offset (in bytes) | Length (in bytes) | Contents
* 0x0 | 0x14 | Session key (address)
* 0x14 | 0x14 | Permitted destination contract (address)
* 0x28 | 0x4 | Permitted selector (bytes4)
* 0x2c | 0x10 | Permitted value limit (uint128)
* 0x3c | 0x2 | Rules list length (uint16)
* 0x3e + 0x23*N | 0x23 | Rule #N
*
* Rule layout:
* Offset (in bytes) | Length (in bytes) | Contents
* 0x0 | 0x2 | Offset (uint16)
* 0x2 | 0x1 | Condition (uint8)
* 0x3 | 0x20 | Value (bytes32)
*
* Condition is a uint8, and can be one of the following:
* 0: EQUAL
* 1: LESS_THAN_OR_EQUAL
* 2: LESS_THAN
* 3: GREATER_THAN_OR_EQUAL
* 4: GREATER_THAN
* 5: NOT_EQUAL
*
* Inspired by https://github.com/zerodevapp/kernel/blob/main/src/validator/SessionKeyValidator.sol
*/
contract ABISessionValidationModule is ISessionValidationModule {
/**
* @dev validates if the _op (UserOperation) matches the SessionKey permissions
* and that _op has been signed by this SessionKey
* Please mind the decimals of your exact token when setting maxAmount
* @param _op User Operation to be validated.
* @param _userOpHash Hash of the User Operation to be validated.
* @param _sessionKeyData SessionKey data, that describes sessionKey permissions
* @param _sessionKeySignature Signature over the the _userOpHash.
* @return true if the _op is valid, false otherwise.
*/
function validateSessionUserOp(
UserOperation calldata _op,
bytes32 _userOpHash,
bytes calldata _sessionKeyData,
bytes calldata _sessionKeySignature
) external pure override returns (bool) {
require(
bytes4(_op.callData[0:4]) == EXECUTE_OPTIMIZED_SELECTOR ||
bytes4(_op.callData[0:4]) == EXECUTE_SELECTOR,
"ABISV Invalid Selector"
);

bytes calldata callData = _op.callData;
address destContract;
uint256 callValue;
bytes calldata data;
assembly {
destContract := calldataload(add(callData.offset, 0x4))
callValue := calldataload(add(callData.offset, 0x24))

let dataOffset := add(
add(callData.offset, 0x04),
calldataload(add(callData.offset, 0x44))
)

let length := calldataload(dataOffset)
data.offset := add(dataOffset, 0x20)
data.length := length
}

return
_validateSessionParams(
destContract,
callValue,
data,
_sessionKeyData
) ==
ECDSA.recover(
ECDSA.toEthSignedMessageHash(_userOpHash),
_sessionKeySignature
);
}

/**
* @dev validates that the call (destinationContract, callValue, funcCallData)
* complies with the Session Key permissions represented by sessionKeyData
* @param destinationContract address of the contract to be called
* @param callValue value to be sent with the call
* @param _funcCallData the data for the call. is parsed inside the SVM
* @param _sessionKeyData SessionKey data, that describes sessionKey permissions
* param _callSpecificData additional data, specific to the call, not used here
* @return sessionKey address of the sessionKey that signed the userOp
* for example to store a list of allowed tokens or receivers
filmakarov marked this conversation as resolved.
Show resolved Hide resolved
*/
function validateSessionParams(
address destinationContract,
uint256 callValue,
bytes calldata _funcCallData,
bytes calldata _sessionKeyData,
bytes memory /*_callSpecificData*/
) external pure virtual override returns (address) {
return
_validateSessionParams(
destinationContract,
callValue,
_funcCallData,
_sessionKeyData
);
}

/**
* @dev validates that the call (destinationContract, callValue, funcCallData)
* complies with the Session Key permissions represented by sessionKeyData
* @param destinationContract address of the contract to be called
* @param callValue value to be sent with the call
* @param _funcCallData the data for the call. is parsed inside the SVM
* @param _sessionKeyData SessionKey data, that describes sessionKey permissions
* @return sessionKey address of the sessionKey that signed the userOp
* for example to store a list of allowed tokens or receivers
*/
function _validateSessionParams(
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add more comments on solidity assembly blocks about what it achieves.
also would be great is constant numbers can be defined as constants or some offset constant then x:x+4 (depending on bytes)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added couple of constants, in other places just added comments not have constant+constant (which would add gas) instead of just hardcoded value

address destinationContract,
uint256 callValue,
bytes calldata _funcCallData,
bytes calldata _sessionKeyData
) internal pure virtual returns (address) {
address sessionKey = address(bytes20(_sessionKeyData[0:20]));
address permittedDestinationContract = address(
bytes20(_sessionKeyData[20:40])
);
bytes4 permittedSelector = bytes4(_sessionKeyData[40:44]);
uint256 permittedValueLimit = uint256(
uint128(bytes16(_sessionKeyData[44:60]))
);
uint256 rulesListLength = uint256(
uint16(bytes2(_sessionKeyData[60:62]))
);

if (destinationContract != permittedDestinationContract) {
revert("ABISV Wrong Destination");
filmakarov marked this conversation as resolved.
Show resolved Hide resolved
}

if (bytes4(_funcCallData[0:4]) != permittedSelector) {
revert("ABISV Wrong Selector");
}

if (callValue > permittedValueLimit) {
revert("ABISV Value exceeded");
}

if (
!_checkRulesForPermission(
_funcCallData,
rulesListLength,
bytes(_sessionKeyData[62:])
)
) {
revert("ABISV Permission rule violated");
}

return sessionKey;
}

/**
* @dev checks if the calldata matches the permission
* @param data the data for the call. is parsed inside the SVM
* @param rulesListLength the length of the rules list
* @param rules the rules list
* @return true if the calldata matches the permission, false otherwise
*/
function _checkRulesForPermission(
bytes calldata data,
uint256 rulesListLength,
bytes calldata rules
) internal pure returns (bool) {
for (uint256 i; i < rulesListLength; ++i) {
(uint256 offset, uint256 condition, bytes32 value) = _parseRule(
rules,
i
);

bytes32 param = bytes32(data[4 + offset:4 + offset + 32]);

bool rulePassed;
assembly ("memory-safe") {
switch condition
case 0 {
// Condition.EQUAL
rulePassed := eq(param, value)
}
case 1 {
// Condition.LESS_THAN_OR_EQUAL
rulePassed := or(lt(param, value), eq(param, value))
}
case 2 {
// Condition.LESS_THAN
rulePassed := lt(param, value)
}
case 3 {
// Condition.GREATER_THAN_OR_EQUAL
rulePassed := or(gt(param, value), eq(param, value))
}
case 4 {
// Condition.GREATER_THAN
rulePassed := gt(param, value)
}
case 5 {
// Condition.NOT_EQUAL
rulePassed := not(eq(param, value))
}
}

if (!rulePassed) {
return false;
}
}
return true;
}

function _parseRule(
filmakarov marked this conversation as resolved.
Show resolved Hide resolved
bytes calldata rules,
uint256 index
) internal pure returns (uint256 offset, uint256 condition, bytes32 value) {
offset = uint256(uint16(bytes2(rules[index * 35:index * 35 + 2])));
condition = uint256(
uint8(bytes1(rules[index * 35 + 2:index * 35 + 3]))
);
value = bytes32(rules[index * 35 + 3:index * 35 + 35]);
}
}
19 changes: 19 additions & 0 deletions contracts/smart-account/test/mocks/MockProtocol/MockProtocol.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,26 @@ pragma solidity ^0.8.23;
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

contract MockProtocol {
mapping(address => uint256) public states;
mapping(address => bytes) public bytesStates;

function interact(address token, uint256 amount) external {
IERC20(token).transferFrom(msg.sender, address(this), amount);
}

function changeState(
uint256 newValue,
bytes calldata bytesValue
) external payable {
states[msg.sender] = newValue;
bytesStates[msg.sender] = bytesValue;
}

function getState(address user) external view returns (uint256) {
return states[user];
}

function getBytesState(address user) external view returns (bytes memory) {
return bytesStates[user];
}
}
2 changes: 1 addition & 1 deletion lib/registry
Loading
Loading