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 33 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
4 changes: 3 additions & 1 deletion .solhintignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,6 @@ 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
contracts/smart-account/test/mocks/MockProtocol.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,268 @@
// 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:
* 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 {
uint256 private constant RULE_LENGTH = 35;
uint256 private constant SELECTOR_LENGTH = 4;

/**
* @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) {
bytes calldata callData = _op.callData;

require(
bytes4(callData[0:4]) == EXECUTE_OPTIMIZED_SELECTOR ||
bytes4(callData[0:4]) == EXECUTE_SELECTOR,
"ABISV Not Execute Selector"
);

address destContract;
uint256 callValue;
bytes calldata data;
assembly {
//offset of the first 32-byte arg is 0x4
destContract := calldataload(add(callData.offset, SELECTOR_LENGTH))
//offset of the second 32-byte arg is 0x24 = 0x4 (SELECTOR_LENGTH) + 0x20 (first 32-byte arg)
callValue := calldataload(add(callData.offset, 0x24))

//we get the data offset from the calldata itself, so no assumptions are made about the data layout
let dataOffset := add(
add(callData.offset, 0x04),
//offset of the bytes arg is stored after selector and two first 32-byte args
// 0x4+0x20+0x20=0x44
calldataload(add(callData.offset, 0x44))
)

let length := calldataload(dataOffset)
//data itself starts after the length which is another 32bytes word, so we add 0x20
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) {
// every address is 20bytes
address sessionKey = address(bytes20(_sessionKeyData[0:20]));
address permittedDestinationContract = address(
bytes20(_sessionKeyData[20:40])
);
// every selector is 4bytes
bytes4 permittedSelector = bytes4(_sessionKeyData[40:44]);
// value limit is encoded as uint128 which is 16 bytes length
uint256 permittedValueLimit = uint256(
uint128(bytes16(_sessionKeyData[44:60]))
);
// rules list length is encoded as uint16 which is 2 bytes length
uint256 rulesListLength = uint256(
uint16(bytes2(_sessionKeyData[60:62]))
);

if (destinationContract != permittedDestinationContract) {
revert("ABISV Destination Forbidden");
}

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

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

if (
!_checkRulesForPermission(
_funcCallData,
rulesListLength,
bytes(_sessionKeyData[62:]) //the rest of the _sessionKeyData is the rules list
)
) {
revert("ABISV Arg 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
);

// get the 32bytes word to verify against reference value from the actual calldata of the userOp
bytes32 param = bytes32(
data[SELECTOR_LENGTH + offset:SELECTOR_LENGTH + 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 := iszero(eq(param, value))
}
}

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

/**
* @dev Parses a rule with a given index from the rules list
* @param rules the rules list as a bytes array
* @param index the index of the rule to be parsed
* @return offset - the offset of the parameter in the calldata (multiplier of 32)
* @return condition - the condition to be checked
* @return value - the reference value to be checked against
*/
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 length is 2 bytes
offset = uint256(
uint16(bytes2(rules[index * RULE_LENGTH:index * RULE_LENGTH + 2]))
);
// condition length is 1 byte
condition = uint256(
uint8(
bytes1(rules[index * RULE_LENGTH + 2:index * RULE_LENGTH + 3])
)
);
// value length is 32 bytes
value = bytes32(
rules[index * RULE_LENGTH + 3:index * RULE_LENGTH + RULE_LENGTH]
);
}
}
36 changes: 36 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,43 @@ 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;
mapping(address => uint256) public unallowedTriggers;

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];
}

function notAllowedMethod() external {
unallowedTriggers[msg.sender]++;
}

function getUnallowedTriggers(
address user
) external view returns (uint256) {
return unallowedTriggers[user];
}

function testArgsMethod(
uint256 arg1,
uint256 arg2,
uint256 arg3
) external {}
}
2 changes: 1 addition & 1 deletion lib/registry
Loading
Loading