Skip to content

Commit

Permalink
refactor: improve code quality
Browse files Browse the repository at this point in the history
  • Loading branch information
vladbochok committed Nov 20, 2024
1 parent bb1973f commit 739f928
Show file tree
Hide file tree
Showing 8 changed files with 113 additions and 204 deletions.
5 changes: 0 additions & 5 deletions packages/contracts/src/AAFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import "@matterlabs/zksync-contracts/l2/system-contracts/libraries/SystemContrac
import { ISsoAccount } from "./interfaces/ISsoAccount.sol";
import { UpgradeableBeacon } from "./UpgradeableBeacon.sol";

import "./helpers/Logger.sol";

contract AAFactory is UpgradeableBeacon {
event AccountCreated(address indexed accountAddress, string uniqueAccountId);

Expand Down Expand Up @@ -43,9 +41,6 @@ contract AAFactory is UpgradeableBeacon {

(accountAddress) = abi.decode(returnData, (address));

Logger.logString("accountAddress");
Logger.logAddress(accountAddress);

// add session-key/spend-limit module (similar code)
ISsoAccount(accountAddress).initialize(initialValidators, initialModules, initialK1Owners);

Expand Down
267 changes: 107 additions & 160 deletions packages/contracts/src/SsoAccount.sol

Large diffs are not rendered by default.

15 changes: 2 additions & 13 deletions packages/contracts/src/batch/BatchCaller.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,12 @@ contract BatchCaller {
function batchCall(Call[] calldata calls) external payable {
require(msg.sender == address(this), "External calls not allowed");

bool isDelegateCall = SystemContractHelper.getCodeAddress() != address(this);
if (!isDelegateCall) {
revert Errors.ONLY_DELEGATECALL();
}

// Execute each call
uint256 len = calls.length;
uint256 totalValue = 0;
Call calldata calli;
for (uint256 i = 0; i < len; ) {
calli = calls[i];
address target = calli.target;
uint256 value = calli.value;
bytes calldata callData = calli.callData;
totalValue += calli.value;

bool success = EfficientCall.rawCall(gasleft(), target, value, callData, false);
totalValue += calls[i].value;
bool success = EfficientCall.rawCall(gasleft(), calls[i].target, calls[i].value, calls[i].callData, false);
if (!calls[i].allowFailure && !success) {
revert Errors.CALL_FAILED();
}
Expand Down
3 changes: 0 additions & 3 deletions packages/contracts/src/handlers/ValidationHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import { ValidatorManager } from "../managers/ValidatorManager.sol";
import { IK1Validator, IR1Validator } from "../interfaces/IValidator.sol";
import { IModuleValidator } from "../interfaces/IModuleValidator.sol";

import "../helpers/Logger.sol";

/**
* @title ValidationHandler
* @notice Contract which calls validators for signature validation
Expand Down Expand Up @@ -47,7 +45,6 @@ abstract contract ValidationHandler is OwnerManager, ValidatorManager {
return true;
}
} else if (_isModuleValidator(validator)) {
Logger.logString("_isModuleValidator");
return IModuleValidator(validator).handleValidation(signedHash, signature);
}

Expand Down
1 change: 1 addition & 0 deletions packages/contracts/src/libraries/Errors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ library Errors {
error FEE_PAYMENT_FAILED();
error UNAUTHORIZED_OUTSIDE_TRANSACTION();
error VALIDATION_HOOK_FAILED();
error METHOD_NOT_IMPLEMENTED();

/*//////////////////////////////////////////////////////////////
LINKED LIST
Expand Down
5 changes: 3 additions & 2 deletions packages/contracts/src/managers/HookManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,15 @@ abstract contract HookManager is IHookManager, Auth {
mapping(address => address) storage validationHooks = _validationHooksLinkedList();

address cursor = validationHooks[AddressLinkedList.SENTINEL_ADDRESS];
uint256 idx = 0;
uint256 idx;
// Iterate through hooks
while (cursor > AddressLinkedList.SENTINEL_ADDRESS) {
// Call it with corresponding hookData
bool success = _call(
cursor,
abi.encodeWithSelector(IValidationHook.validationHook.selector, signedHash, transaction, hookData[idx++])
abi.encodeCall(IValidationHook.validationHook, (signedHash, transaction, hookData[idx]))
);
++idx;

if (!success) {
return false;
Expand Down
2 changes: 0 additions & 2 deletions packages/contracts/src/validators/PasskeyValidator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import { VerifierCaller } from "../helpers/VerifierCaller.sol";
import { JsmnSolLib } from "../libraries/JsmnSolLib.sol";
import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";

import "../helpers/Logger.sol";

/**
* @title validator contract for passkey r1 signatures
* @author https://getclave.io
Expand Down
19 changes: 0 additions & 19 deletions packages/contracts/src/validators/WebAuthValidator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ pragma solidity ^0.8.24;
import { IModuleValidator } from "../interfaces/IModuleValidator.sol";
import "./PasskeyValidator.sol";

import "../helpers/Logger.sol";

/**
* @title validator contract for passkey r1 signatures
* @author https://getclave.io
Expand All @@ -29,10 +27,6 @@ contract WebAuthValidator is PasskeyValidator, IModuleValidator {
}

function handleValidation(bytes32 signedHash, bytes memory signature) external view returns (bool) {
// Printing this hash makes capturing this for a replay test easier
Logger.logString("signed hash");
Logger.logBytes32(signedHash);

return webAuthVerify(signedHash, signature);
}

Expand All @@ -42,22 +36,18 @@ contract WebAuthValidator is PasskeyValidator, IModuleValidator {
);

if (rs[1] > lowSmax) {
Logger.logString("malleability check failed");
return false;
}

// check if the flags are set
if (authenticatorData[32] & AUTH_DATA_MASK != AUTH_DATA_MASK) {
Logger.logString("auth data mask failed");
return false;
}

// parse out the important fields (type, challenge, and origin): https://goo.gl/yabPex
// TODO: test if the parse fails for more than 10 elements, otherwise can have a malicious header
(uint returnValue, JsmnSolLib.Token[] memory tokens, uint actualNum) = JsmnSolLib.parse(clientDataJSON, 20);
if (returnValue != 0) {
Logger.logString("failed to parse json");
Logger.logUint(returnValue);
return false;
}

Expand All @@ -76,38 +66,30 @@ contract WebAuthValidator is PasskeyValidator, IModuleValidator {
string memory challengeValue = JsmnSolLib.getBytes(clientDataJSON, nextT.start, nextT.end);
// this should only be set once, otherwise this is an error
if (validChallenge) {
Logger.logString("duplicate challenge, bad json!");
return false;
}
// this is the key part to ensure the signature is for the provided transaction
bytes memory challengeDataArray = Base64.decode(challengeValue);
if (challengeDataArray.length != 32) {
// wrong hash size
Logger.logString("invalid hash data length in json challenge field");
return false;
}
bytes32 challengeData = abi.decode(challengeDataArray, (bytes32));

validChallenge = challengeData == transactionHash;
Logger.logString("validChallenge");
Logger.logBool(validChallenge);
} else if (Strings.equal(keyOrValue, "type")) {
JsmnSolLib.Token memory nextT = tokens[index + 1];
string memory typeValue = JsmnSolLib.getBytes(clientDataJSON, nextT.start, nextT.end);
// this should only be set once, otherwise this is an error
if (validType) {
Logger.logString("duplicate type field, bad json");
return false;
}
validType = Strings.equal("webauthn.get", typeValue);
Logger.logString("valid type");
Logger.logBool(validType);
} else if (Strings.equal(keyOrValue, "origin")) {
JsmnSolLib.Token memory nextT = tokens[index + 1];
string memory originValue = JsmnSolLib.getBytes(clientDataJSON, nextT.start, nextT.end);
// this should only be set once, otherwise this is an error
if (validOrigin) {
Logger.logString("duplicate origin field, bad json");
return false;
}
pubKey[0] = lowerKeyHalf[originValue][msg.sender];
Expand All @@ -121,7 +103,6 @@ contract WebAuthValidator is PasskeyValidator, IModuleValidator {
}

if (!validChallenge || !validType) {
Logger.logString("invalid challenge or type");
return false;
}

Expand Down

0 comments on commit 739f928

Please sign in to comment.