Skip to content

audit fixes #80

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.

Sign up for GitHub

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

Merged
leekt merged 2 commits into from
Feb 6, 2024
Merged

audit fixes #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cea9e64
audit fixes
leekt Feb 5, 2024
9bc9cc6
fix: update prevSigner to signer on validateSignature and added invar…
leekt Feb 6, 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
64 changes: 35 additions & 29 deletions src/validator/WeightedECDSAValidator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,34 +51,45 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator {
event GuardianRemoved(address indexed guardian, address indexed kernel);

function _domainNameAndVersion() internal pure override returns (string memory, string memory) {
return ("WeightedECDSAValidator", "0.0.2");
return ("WeightedECDSAValidator", "0.0.3");
}

function enable(bytes calldata _data) external payable override {
(address[] memory _guardians, uint24[] memory _weights, uint24 _threshold, uint48 _delay) =
abi.decode(_data, (address[], uint24[], uint24, uint48));
function _addGuardians(address[] memory _guardians, uint24[] memory _weights, address _kernel) internal {
uint24 totalWeight = weightedStorage[_kernel].totalWeight;
require(_guardians.length == _weights.length, "Length mismatch");
require(weightedStorage[msg.sender].totalWeight == 0, "Already enabled");
weightedStorage[msg.sender].firstGuardian = msg.sender;
uint160 prevGuardian = uint160(weightedStorage[_kernel].firstGuardian);
for (uint256 i = 0; i < _guardians.length; i++) {
require(_guardians[i] != msg.sender, "Guardian cannot be self");
require(_guardians[i] != _kernel, "Guardian cannot be self");
require(_guardians[i] != address(0), "Guardian cannot be 0");
require(_weights[i] != 0, "Weight cannot be 0");
require(guardian[_guardians[i]][msg.sender].weight == 0, "Guardian already enabled");
guardian[_guardians[i]][msg.sender] =
GuardianStorage({weight: _weights[i], nextGuardian: weightedStorage[msg.sender].firstGuardian});
weightedStorage[msg.sender].firstGuardian = _guardians[i];
weightedStorage[msg.sender].totalWeight += _weights[i];
emit GuardianAdded(_guardians[i], msg.sender, _weights[i]);
require(guardian[_guardians[i]][_kernel].weight == 0, "Guardian already enabled");
require(uint160(_guardians[i]) < prevGuardian, "Guardians not sorted");
guardian[_guardians[i]][_kernel] =
GuardianStorage({weight: _weights[i], nextGuardian: weightedStorage[_kernel].firstGuardian});
weightedStorage[_kernel].firstGuardian = _guardians[i];
totalWeight += _weights[i];
prevGuardian = uint160(_guardians[i]);
emit GuardianAdded(_guardians[i], _kernel, _weights[i]);
}
weightedStorage[_kernel].totalWeight = totalWeight;
}

function enable(bytes calldata _data) external payable override {
(address[] memory _guardians, uint24[] memory _weights, uint24 _threshold, uint48 _delay) =
abi.decode(_data, (address[], uint24[], uint24, uint48));
require(_guardians.length == _weights.length, "Length mismatch");
require(weightedStorage[msg.sender].totalWeight == 0, "Already enabled");
weightedStorage[msg.sender].firstGuardian = address(uint160(type(uint160).max));
_addGuardians(_guardians, _weights, msg.sender);
weightedStorage[msg.sender].delay = _delay;
weightedStorage[msg.sender].threshold = _threshold;
require(_threshold <= weightedStorage[msg.sender].totalWeight, "Threshold too high");
}

function disable(bytes calldata) external payable override {
require(weightedStorage[msg.sender].totalWeight != 0, "Not enabled");
address currentGuardian = weightedStorage[msg.sender].firstGuardian;
while (currentGuardian != msg.sender) {
while (currentGuardian != address(uint160(type(uint160).max))) {
address nextGuardian = guardian[currentGuardian][msg.sender].nextGuardian;
emit GuardianRemoved(currentGuardian, msg.sender);
delete guardian[currentGuardian][msg.sender];
Expand All @@ -101,18 +112,8 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator {
}
delete weightedStorage[msg.sender];
require(_guardians.length == _weights.length, "Length mismatch");
weightedStorage[msg.sender].firstGuardian = msg.sender;
for (uint256 i = 0; i < _guardians.length; i++) {
require(_guardians[i] != msg.sender, "Guardian cannot be self");
require(_guardians[i] != address(0), "Guardian cannot be 0");
require(_weights[i] != 0, "Weight cannot be 0");
require(guardian[_guardians[i]][msg.sender].weight == 0, "Guardian already enabled");
guardian[_guardians[i]][msg.sender] =
GuardianStorage({weight: _weights[i], nextGuardian: weightedStorage[msg.sender].firstGuardian});
weightedStorage[msg.sender].firstGuardian = _guardians[i];
weightedStorage[msg.sender].totalWeight += _weights[i];
emit GuardianAdded(_guardians[i], msg.sender, _weights[i]);
}
weightedStorage[msg.sender].firstGuardian = _guardians[0];
_addGuardians(_guardians, _weights, msg.sender);
weightedStorage[msg.sender].delay = _delay;
weightedStorage[msg.sender].threshold = _threshold;
}
Expand Down Expand Up @@ -221,13 +222,14 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator {
return packValidationData(ValidAfter.wrap(0), ValidUntil.wrap(0));
}
} else if (proposal.status == ProposalStatus.Approved || passed) {
if (userOp.paymasterAndData.length == 0) {
if (userOp.paymasterAndData.length == 0 || address(bytes20(userOp.paymasterAndData[0:20])) == address(0)) {
address signer = ECDSA.recover(ECDSA.toEthSignedMessageHash(userOpHash), userOp.signature);
if (guardian[signer][msg.sender].weight != 0) {
proposal.status = ProposalStatus.Executed;
return packValidationData(proposal.validAfter, ValidUntil.wrap(0));
}
} else {
proposal.status = ProposalStatus.Executed;
return packValidationData(proposal.validAfter, ValidUntil.wrap(0));
}
}
Expand Down Expand Up @@ -263,13 +265,17 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator {
return SIG_VALIDATION_FAILED;
}
uint256 totalWeight = 0;
address signer;
address prevSigner = address(uint160(type(uint160).max));
for (uint256 i = 0; i < sigCount; i++) {
signer = ECDSA.recover(hash, signature[i * 65:(i + 1) * 65]);
address signer = ECDSA.recover(hash, signature[i * 65:(i + 1) * 65]);
totalWeight += guardian[signer][msg.sender].weight;
if (totalWeight >= strg.threshold) {
return packValidationData(ValidAfter.wrap(0), ValidUntil.wrap(0));
}
if (signer >= prevSigner) {
return SIG_VALIDATION_FAILED;
}
prevSigner = signer;
}
return SIG_VALIDATION_FAILED;
}
Expand Down
43 changes: 27 additions & 16 deletions test/foundry/validator/WeightedECDSAValidator.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,19 @@ contract KernelWeightedECDSATest is KernelTestBase {
(owners[0], ownerKeys[0]) = makeAddrAndKey("owner0");
(owners[1], ownerKeys[1]) = makeAddrAndKey("owner1");
(owners[2], ownerKeys[2]) = makeAddrAndKey("owner2");
// sort owners and keys from largest to smallest owner address
for (uint256 i = 0; i < owners.length; i++) {
for (uint256 j = i + 1; j < owners.length; j++) {
if (owners[i] < owners[j]) {
address tempAddr = owners[i];
owners[i] = owners[j];
owners[j] = tempAddr;
uint256 tempKey = ownerKeys[i];
ownerKeys[i] = ownerKeys[j];
ownerKeys[j] = tempKey;
}
}
}
weights = [uint24(1), uint24(2), uint24(3)];
threshold = 3;
delay = 0;
Expand Down Expand Up @@ -84,7 +97,7 @@ contract KernelWeightedECDSATest is KernelTestBase {
abi.encode(
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
keccak256("WeightedECDSAValidator"),
keccak256("0.0.2"),
keccak256("0.0.3"),
block.chainid,
address(defaultValidator)
)
Expand Down Expand Up @@ -120,9 +133,9 @@ contract KernelWeightedECDSATest is KernelTestBase {
}

function getWrongSignature(bytes32 hash) internal view override returns (bytes memory) {
(uint8 v0, bytes32 r0, bytes32 s0) = vm.sign(ownerKeys[0], hash);
(uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(ownerKeys[1] + 1, hash);
(uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(ownerKeys[2] + 1, hash);
(uint8 v0, bytes32 r0, bytes32 s0) = vm.sign(ownerKeys[1], hash);
(uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(ownerKeys[0], hash);
(uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(ownerKeys[2], hash);
return abi.encodePacked(r0, s0, v0, r1, s1, v1, r2, s2, v2);
}

Expand All @@ -142,17 +155,15 @@ contract KernelWeightedECDSATest is KernelTestBase {
}

function test_default_validator_disable() external override {
//UserOperation memory op = buildUserOperation(
// abi.encodeWithSelector(
// IKernel.execute.selector,
// address(defaultValidator),
// 0,
// abi.encodeWithSelector(ECDSAValidator.disable.selector, ""),
// Operation.Call
// )
//);
//performUserOperationWithSig(op);
//(address owner) = ECDSAValidator(address(defaultValidator)).ecdsaValidatorStorage(address(kernel));
//assertEq(owner, address(0), "owner should be 0");
UserOperation memory op = buildUserOperation(
abi.encodeWithSelector(
IKernel.execute.selector,
address(defaultValidator),
0,
abi.encodeWithSelector(WeightedECDSAValidator.disable.selector, ""),
Operation.Call
)
);
performUserOperationWithSig(op);
}
}