Add OR logic to calldata hashes

prateek105 commented 3 weeks ago

File	% Lines	% Statements	% Branches	% Funcs
src/BytesHelper.sol	100.00% (13/13)	100.00% (18/18)	100.00% (10/10)	100.00% (4/4)
src/ConfigurablePauseGuardian.sol	100.00% (20/20)	100.00% (22/22)	100.00% (6/6)	100.00% (7/7)
src/Guard.sol	100.00% (8/8)	100.00% (9/9)	100.00% (10/10)	100.00% (3/3)
src/InstanceDeployer.sol	100.00% (53/53)	100.00% (66/66)	62.50% (10/16)	100.00% (2/2)
src/RecoverySpell.sol	100.00% (49/49)	100.00% (66/66)	100.00% (21/21)	100.00% (7/7)
src/RecoverySpellFactory.sol	100.00% (19/19)	100.00% (27/27)	100.00% (15/15)	100.00% (3/3)
src/Timelock.sol	100.00% (186/186)	100.00% (236/236)	97.00% (97/100)	100.00% (48/48)
src/TimelockFactory.sol	100.00% (4/4)	100.00% (4/4)	100.00% (0/0)	100.00% (2/2)
src/deploy/SystemDeploy.s.sol	90.91% (30/33)	92.31% (36/39)	50.00% (6/12)	0.00% (0/5)
src/views/AddressCalculation.sol	100.00% (24/24)	100.00% (29/29)	100.00% (8/8)	100.00% (3/3)

Timelock coverage is 100% for lines, functions and statements. 97% for branches, only the branches where assert has been added for formal verification are missing.

Used following command for running coverage: forge coverage -vv --fork-url https://eth-mainnet.g.alchemy.com/v2/<key>

ElliotFriedman commented 3 weeks ago

Compiler warnings that would be great to clean up

ElliotFriedman commented 3 weeks ago

Slither output:

Timelock._addCalldataCheck(address,bytes4,uint16,uint16,bytes[],bool[]) (src/Timelock.sol#994-1083) ignores return value by indexes[indexLength].dataHashes.add(dataHash) (src/Timelock.sol#1073)
Timelock._removeCalldataCheck(address,bytes4,uint256) (src/Timelock.sol#1124-1167) ignores return value by indexCheck.dataHashes.remove(removedDataHashes[i]) (src/Timelock.sol#1146)
Timelock._removeCalldataCheck(address,bytes4,uint256) (src/Timelock.sol#1124-1167) ignores return value by indexCheck.dataHashes.add(dataHashes[i_scope_0]) (src/Timelock.sol#1154)
Timelock._removeCalldataCheck(address,bytes4,uint256) (src/Timelock.sol#1124-1167) ignores return value by lastIndexCheck.dataHashes.remove(dataHashes[i_scope_0]) (src/Timelock.sol#1155)
Timelock._removeAllCalldataChecks(address,bytes4) (src/Timelock.sol#1176-1210) ignores return value by removedCalldataCheck.dataHashes.remove(dataHashes[i]) (src/Timelock.sol#1202)

All of these locations should check the return value and assert the value is true.

ElliotFriedman commented 3 weeks ago

Slither output:

Timelock._addCalldataCheck(address,bytes4,uint16,uint16,bytes[],bool[]) (src/Timelock.sol#994-1083) ignores return value by indexes[indexLength].dataHashes.add(dataHash) (src/Timelock.sol#1073)
Timelock._removeCalldataCheck(address,bytes4,uint256) (src/Timelock.sol#1124-1167) ignores return value by indexCheck.dataHashes.remove(removedDataHashes[i]) (src/Timelock.sol#1146)
Timelock._removeCalldataCheck(address,bytes4,uint256) (src/Timelock.sol#1124-1167) ignores return value by indexCheck.dataHashes.add(dataHashes[i_scope_0]) (src/Timelock.sol#1154)
Timelock._removeCalldataCheck(address,bytes4,uint256) (src/Timelock.sol#1124-1167) ignores return value by lastIndexCheck.dataHashes.remove(dataHashes[i_scope_0]) (src/Timelock.sol#1155)
Timelock._removeAllCalldataChecks(address,bytes4) (src/Timelock.sol#1176-1210) ignores return value by removedCalldataCheck.dataHashes.remove(dataHashes[i]) (src/Timelock.sol#1202)

All of these locations should check the return value and assert the value is true.

Please try re-running slither and seeing if these issues are gone.

prateek105 commented 2 weeks ago

Please try re-running slither and seeing if these issues are gone.

Checked. These issues are gone.

Slither output in red and yellow colour:

    Dangerous calls:
    - require(bool,string)(receiver.send(payment),GS011) (lib/safe-smart-account/contracts/Safe.sol#243)
Timelock._execute(address,uint256,bytes) (src/Timelock.sol#980-985) sends eth to arbitrary user
    Dangerous calls:
    - (success) = target.call{value: value}(data) (src/Timelock.sol#983)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#functions-that-send-ether-to-arbitrary-destinations

IERC165 is re-used:
    - IERC165 (lib/openzeppelin-contracts/contracts/utils/introspection/IERC165.sol#15-25)
    - IERC165 (lib/safe-smart-account/contracts/interfaces/IERC165.sol#5-15)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#name-reused

Reentrancy in Timelock.execute(address,uint256,bytes,bytes32) (src/Timelock.sol#601-621):
    External calls:
    - _execute(target,value,payload) (src/Timelock.sol#614)
        - (success) = target.call{value: value}(data) (src/Timelock.sol#983)
    State variables written after the call(s):
    - _afterCall(id) (src/Timelock.sol#620)
        - timestamps[id] = _DONE_TIMESTAMP (src/Timelock.sol#973)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities

Timelock.isOperationDone(bytes32) (src/Timelock.sol#422-424) uses a dangerous strict equality:
    - timestamps[id] == _DONE_TIMESTAMP (src/Timelock.sol#423)
ConfigurablePauseGuardian.paused() (src/ConfigurablePauseGuardian.sol#76-80) uses a dangerous strict equality:
    - pauseStartTime == 0 (src/ConfigurablePauseGuardian.sol#77-79)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-strict-equalities

AccessControlEnumerable._grantRole(bytes32,address) (lib/openzeppelin-contracts/contracts/access/extensions/AccessControlEnumerable.sol#64-70) ignores return value by _roleMembers[role].add(account) (lib/openzeppelin-contracts/contracts/access/extensions/AccessControlEnumerable.sol#67)
AccessControlEnumerable._revokeRole(bytes32,address) (lib/openzeppelin-contracts/contracts/access/extensions/AccessControlEnumerable.sol#75-81) ignores return value by _roleMembers[role].remove(account) (lib/openzeppelin-contracts/contracts/access/extensions/AccessControlEnumerable.sol#78)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unused-return

solidity-labs-io / kleidi

Add OR logic to calldata hashes #20