Farseen0/scar-pairs huggingface dataset:
LOW severity: `SwapOperations` computes the swap fees without accounting for how fees will alter reserves. **Impact**

At a broadstroke The formula to compute the post-swap reserves used by `getAmountsOut` and `getAmountsIn` is as follows:

However, the math is not accounting for fees that will be taken on each swap, meaning that the post-swap reserves will not match this amount

**Mitigation**

You...

Vulnerable code:
uint(reserveA) * reserveB) / (reserveB + amtB),
HIGH severity: `ERPermit` can be front-runned and should be try/catched. **Impact**

Permit can be broadcasted by anyone and will revert if using the incorrect nonce

Due to this you should use try/catch it

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/BorrowerOperations.sol#L225-L245

https://github.com/bl...

Vulnerable code:
function openTroveWithPermit(
    TokenAmount[] memory _colls,
    bytes[] memory _priceUpdateData,
    uint deadline,
    uint8[] memory v,
    bytes32[] memory r,
    bytes32[] memory s
  ) external payable {
    for (uint i = 0; i < _colls.length; i++)
      IERC20Permit(_colls[i].tokenAddress).permit( /// @audit Should use Permit
        msg.sender,
        address(this),
        _colls[i].amount,
        deadline,
        v[i],
        r[i],
        s[i]
      );

    openTrove(_colls, _priceUpdateData);
  }

IERC20Permit(tokenA).permit(msg.sender, address(this), amountADesired, deadline, v[0], r[0], s[0]);
    IERC20Permit(tokenB).permit(msg.sender, address(this), amountBDesired, deadline, v[1], r[1], s[1]);

IERC20Permit(path[0]).permit(msg.sender, address(this), amountIn, deadline, v, r, s);
LOW severity: `SwapPair` events use `msg.sender` but they should use the user taking on debt. **Impact**

Events in SwapPair are logging `msg.sender` which will always be `swapOperations`

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/SwapPair.sol#L125-L126

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9be...

Vulnerable code:
emit Mint(msg.sender, amount0, amount1); /// @audit QA: Mint should change to `to`

emit Burn(msg.sender, amount0, amount1, to); /// @audit QA: Mint should change to `to` or to the cdp Owner
LOW severity: `SwapPair` may revert on 0 transfer tokens. **Impact**
Certain tokens revert on a 0 amount transfer

The code also performs the check to prevent zero transfers in many parts, but in these 2 instances the check was missed

[Note that for OZ tokens, the check is not...

Vulnerable code:
if (amount0InFee > 0) {
      uint amount0GovFee = (amount0InFee * swapOperations.getGovSwapFee()) / DECIMAL_PRECISION;
      _safeTransfer(token0, tokenManager.govPayoutAddress(), amount0GovFee); /// @audit 0 revert
      balance0 -= amount0GovFee;
    }

// payout whats left
    _safeTransfer(token0, to, amount0 - burned0);
    _safeTransfer(token1, to, amount1 - burned1);
LOW severity: `SwapPair` is intended to overflow, but is not using `unchecked`. **Overflow logic from UniV2 no longer applies**

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/SwapPair.sol#L84-L93

The code asserts that `+= overflow is desired` but this cannot happen on the current solidity version
This means that.....

Vulnerable code:
function _update(uint balance0, uint balance1, uint112 _reserve0, uint112 _reserve1) private {
    if (balance0 > type(uint112).max || balance1 > type(uint112).max) revert Overflow();

    uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);
    uint32 timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
    if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
      // * never overflows, and + overflow is desired | /// @audit Overflow is wrong, this is solidity 0.8!!
      price0CumulativeLast += uint(UQ112x112.encode(_reserve1).uqdiv(_reserve0)) * timeElapsed;
      price1CumulativeLast += uint(UQ112x112.encode(_reserve0).uqdiv(_reserve1)) * timeElapsed;
    }

uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);

// forge test --match-test test_theOverflow -vv
    function test_theOverflow() public {
        vm.warp(type(uint32).max + 1);
        uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);

    }
LOW severity: Incorrect encoding for Permit. **Impact**

The additional `MessageHashUtils.toEthSignedMessageHash(digest);` is not necessary and is re-encoding the user signature

Per the EIP, you're supposed to simply sign on the digest
https://eips.ethereum.org/EIPS/eip-2612

Also check this implementation of a JS signature...

Vulnerable code:
function permit(
    address owner,
    address spender,
    uint amount,
    uint deadline,
    uint8 v,
    bytes32 r,
    bytes32 s
  ) external override {
    if (deadline < block.timestamp) revert ExpiredDeadline();
    bytes32 digest = keccak256(
      abi.encodePacked(
        '\x19\x01',
        this.domainSeparator(),
        keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, amount, _nonces[owner]++, deadline))
      )
    );

    bytes32 signedMsg = MessageHashUtils.toEthSignedMessageHash(digest); /// @audit this is not necessary /  incorrect, it's a double encoding
    address recoveredAddress = ECDSA.recover(signedMsg, v, r, s); /// @audit QA: This is not Permit EIP!
    if (recoveredAddress != owner) revert InvalidSignature();
    _approve(owner, spender, amount);
  }

function permit(
    address owner,
    address spender,
    uint amount,
    uint deadline,
    uint8 v,
    bytes32 r,
    bytes32 s
  ) external override {
    if (deadline < block.timestamp) revert ExpiredDeadline();
    bytes32 digest = keccak256(
      abi.encodePacked(
        '\x19\x01',
        this.domainSeparator(),
        keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, amount, _nonces[owner]++, deadline))
      )
    );

    address recoveredAddress = ECDSA.recover(digest, v, r, s);
    if (recoveredAddress != owner) revert InvalidSignature();
    _approve(owner, spender, amount);
  }
LOW severity: Operative Gotcha - Safe.getModules is limited to the first 10 modules. **Impact**

`_isModuleEnabled` is written as follows:

https://github.com/onchainification/aura_locker_v2/blob/07294ae3638909ecd768a6a0f831fa513abe91a0/src/AuraLockerModule.sol#L123

Which uses `getModules`, which is paginated and limited to the first 10 enabled...

Vulnerable code:
/// @dev The Gnosis Safe v1.1.1 does not yet have the `isModuleEnabled` method, so we need a workaround
    function _isModuleEnabled() internal view returns (bool) {
        address[] memory modules = SAFE.getModules();
        for (uint256 i = 0; i < modules.length; i++) {
            if (modules[i] == address(this)) return true;
        }
        return false;
    }

function getModules()
        public
        view
        returns (address[] memory)
    {
        (address[] memory array,) = getModulesPaginated(SENTINEL_MODULES, 10);
        return array;
    }
LOW severity: [G-01] Gas Optimization: Can skip check to save 200 gas. **Impact**

`performUpkeep` has a check to verify if there's any re-lockable balance

https://github.com/onchainification/aura_locker_v2/blob/07294ae3638909ecd768a6a0f831fa513abe91a0/src/AuraLockerModule.sol#L112-L113

If the check fails, the call will revert.

However, vlAURA already has this...

Vulnerable code:
/// @notice The actual execution of the action determined by the `checkUpkeep` method (AURA locking)
    function performUpkeep(bytes calldata /* _performData */ ) external override onlyKeeper {
        if (!_isModuleEnabled()) revert ModuleNotEnabled();

        (, uint256 unlockable,,) = AURA_LOCKER.lockedBalances(address(SAFE));
        if (unlockable == 0) revert NothingToLock(block.timestamp);

function _processExpiredLocks(
        address _account,
        bool _relock,
        address _rewardAddress,
        uint256 _checkDelay
    ) internal updateReward(_account) {
/// OMITTED

require(length > 0, "no locks"); /// @audit Reverts here
LOW severity: `Timelock` unnecessary call of `_setRoleAdmin`. The call

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/Timelock.sol#L307-L308

Is unnecessary

Since DEFAULT_ADMIN_ROLE == 0x00

Meaning that it will be admin role by default

Vulnerable code:
/// set the admin of the hot signer role to the default admin role
        _setRoleAdmin(HOT_SIGNER_ROLE, DEFAULT_ADMIN_ROLE); /// @audit prob implict
LOW severity: `Timelock` encoding of bytes is unambigous while strings may cause issues. **Encoding (UI risk)** 

This test fails

This doesn't

Because the first one is converting the bytes to literals
While the second one is converting them from hex, which is consistent with encodePacked values

It's important that while the UI shows user friendly values, that all values passed to...

Vulnerable code:
bytes wrongCheck = "a54D3c09E34aC96807c1CC397404bF2B98DC4eFb";
    bytes rightCheck = "a54d3c09E34aC96807c1CC397404bF2B98DC4eFb";

    function test_bytes_checksum() public {
        bytes32 kak1 = keccak256(wrongCheck);
        bytes32 kak2 = keccak256(rightCheck);

        assertEq(kak1, kak2, "same res");
    }

bytes wrongCheck = hex"a54D3c09E34aC96807c1CC397404bF2B98DC4eFb";
    bytes rightCheck = hex"a54d3c09E34aC96807c1CC397404bF2B98DC4eFb";

    function test_bytes_checksum() public {
        bytes32 kak1 = keccak256(wrongCheck);
        bytes32 kak2 = keccak256(rightCheck);

        assertEq(kak1, kak2, "same res");
    }
LOW severity: `Timelock`  `isSelfAddressCheck` can be removed. https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/Timelock.sol#L1166-L1177

The hash is known at time of setup, so there's no advantage in having this code

Vulnerable code:
if (isSelfAddressCheck[i]) {
                /// self address check, data must be empty
                require(
                    data[i].length == 0,
                    "CalldataList: Data must be empty for self address check"
                );
                require(
                    endIndex - startIndex == 20,
                    "CalldataList: Self address check must be 20 bytes"
                );
                dataHash = ADDRESS_THIS_HASH;
            } else {
LOW severity: `Timelock` checks for `1` instead of `DONE_TIMESTAMP`. This should be `DONE_TIMESTAMP`

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/Timelock.sol#L430-L431

Vulnerable code:
require(timestamp != 1, "Timelock: operation already executed");
LOW severity: `Timelock` prevents multiple replays, but is subject to cross operation reentrancy. **Impact**

The code for `execute` is as follows:

https://github.com/solidity-labs-io/kleidi/blob/a5f6eb7e9eb5870f2f45b4f4b93e1b4da5f5cce1/src/Timelock.sol#L595-L608

Before executing it will remove the `id` from liveProposals, making a double execution of the function impossible

There's a...

Vulnerable code:
bytes32 id = hashOperation(target, value, payload, salt);

        /// first reentrancy check, impossible to reenter and execute the same
        /// proposal twice
        require(_liveProposals.remove(id), "Timelock: proposal does not exist");
        require(isOperationReady(id), "Timelock: operation is not ready");

        _execute(target, value, payload);
        emit CallExecuted(id, 0, target, value, payload);

        /// second reentrancy check, second check that operation is ready,
        /// operation will be not ready if already executed as timestamp will
        /// be set to 1
        _afterCall(id);
LOW severity: `Timelock` `CalldataAdded` will log all `dataHashes` instead of the ones being added. **Event Data**

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/Timelock.sol#L1198-L1199

Vulnerable code:
indexes[targetIndex].dataHashes.values() /// @audit technically adding all, also technically will emit a hash not the actual value
LOW severity: Once a `RecoverySpell` is deployed, all spells on all chains may be deployed. **Impact**

RecoverySpells are meant to be used in a time of emergency

For various reasons a recovery may need to be performed exclusively on one chain

However, calling `createRecoverySpell` will leak key details about the...

Vulnerable code:
function createRecoverySpell(
        bytes32 salt,
        address[] memory owners,
        address safe,
        uint256 threshold,
        uint256 recoveryThreshold,
        uint256 delay
    ) external returns (RecoverySpell recovery) {
LOW severity: `InstanceDeployer` - Could use constant for Flag Previous Owner. Could use CONSTANT instead of `1`

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/InstanceDeployer.sol#L294-L302

Vulnerable code:
calls3[index++].callData = abi.encodeWithSelector(
                OwnerManager.swapOwner.selector,
                /// previous owner
                address(1),
                /// old owner (this address)
                address(this),
                /// new owner, the first owner the caller wants to add
                instance.owners[0]
            );
LOW severity: `ConfigurablePause.sol` some indexed parameters are not particularly useful. **Not sure it makes sense to index this**

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/ConfigurablePause.sol#L46-L47

That would be helpful to find all events at a time, but that's a pretty niche thing to do

Same...

Vulnerable code:
event PauseTimeUpdated(uint256 indexed newPauseStartTime);

event PauseDurationUpdated(
        uint256 indexed oldPauseDuration, uint256 newPauseDuration
    );
LOW severity: Guard QA Report. **Analysis**

Fundamentally enforces that no delegate call can be called from the safe

It can still be called via the Modules and MAYBE via the fallback module

Seems to be upgrade resistant and I don't believe upgrades are "intended"

You cannot guarantee the safe will be empty
But the behaviour.....

Vulnerable code:
/// no fallback handler allowed by Guard
            address(0),

/// Refund receiver and gas params are not checked because the Safe itself
/// does not hold funds or tokens.

function execTransactionFromModule(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation
    ) external override returns (bool success) {
        (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation);
        success = execute(to, value, data, operation, type(uint256).max);
        postModuleExecution(guard, guardHash, success);
    }
LOW severity: Different Threshold can result in different hash but same config when using one owner. **Impact**

When calling `createSystemInstance`, a Safe with a single owner could be deployed, while passing a threshold that is above 1

Because of the check `if (instance.owners.length > 1) {` the threshold will not be validated in those cases

Because `threshold` is part of the salt, this will...

Vulnerable code:
for (uint256 i = 1; i < instance.owners.length - 1; i++) {
                calls3[index++].callData = abi.encodeWithSelector(
                    OwnerManager.addOwnerWithThreshold.selector,
                    instance.owners[i],
                    1
                );
            }

            /// if there is only one owner, the threshold is set to 1
            /// if there are more than one owner, add the final owner with the
            /// updated threshold
            if (instance.owners.length > 1) {
                /// add final owner with the updated threshold
                /// if threshold is greater than the number of owners, that
                /// will be caught in the addOwnerWithThreshold function with
                /// error "GS201"
                calls3[index++].callData = abi.encodeWithSelector(
                    OwnerManager.addOwnerWithThreshold.selector,
                    instance.owners[instance.owners.length - 1],
                    instance.threshold
                );
            }
LOW severity: Refactoring - Roles are not used and could be removed. **Impact**

Overall roles are used only for `DEFAULT_ADMIN_ROLE` (self) and for `HOT_SIGNER_ROLE` because of this, you could simply remove the roles logic and leave a way to add and remove `HOT_SIGNERS`

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/Tim...

Vulnerable code:
function grantRole(bytes32 role, address account)
        public
        override(AccessControl, IAccessControl)
    {
        require(role != DEFAULT_ADMIN_ROLE, "Timelock: cannot grant admin role");
        super.grantRole(role, account);
    }

    /// @notice function to revoke a role from an address
    /// @param role the role to revoke
    /// @param account the address to revoke the role from
    function revokeRole(bytes32 role, address account)
        public
        override(AccessControl, IAccessControl)
    {
        require(
            role != DEFAULT_ADMIN_ROLE, "Timelock: cannot revoke admin role"
        );
        super.revokeRole(role, account);
    }

    /// @notice function to renounce a role
    /// @param role the role to renounce
    /// @param account the address to renounce the role from
    function renounceRole(bytes32 role, address account)
        public
        override(AccessControl, IAccessControl)
    {
        require(
            role != DEFAULT_ADMIN_ROLE, "Timelock: cannot renounce admin role"
        );
        super.renounceRole(role, account);
    }
LOW severity: RecoverySpell Gas Optimizations. **Remove address(0) check, this is already done by OZ.ECDSA**

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/RecoverySpell.sol#L226-L230

**Ownership check can be done in memory more cheaply**

The code change is pretty annoying compared to this, but...

Vulnerable code:
/// will be 0 on the second retrieval and the require will fail.
            require( /// @audit Pretty sure OZ does this
                valid && recoveredAddress != address(0),
                "RecoverySpell: Invalid signature"
            );

assembly ("memory-safe") {
                valid := tload(recoveredAddress)
                if eq(valid, 1) { tstore(recoveredAddress, 0) } /// @audit Cannot recover more than once per address
            }
LOW severity: `getFunctionSignature` can be refactored to not use assembly. **Impact**

`getFunctionSignature` uses assembly as follows

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/BytesHelper.sol#L7-L18

But you can simply cast bytes to bytes4 instead

**Proof Of Concept**

Vulnerable code:
function getFunctionSignature(bytes memory toSlice)
        public
        pure
        returns (bytes4 functionSignature)
    {
        require(toSlice.length >= 4, "No function signature");

        assembly ("memory-safe") {
            functionSignature := mload(add(toSlice, 0x20))
        }
        return bytes4(toSlice);
    }

function getFunctionSignature(bytes memory toSlice)
        public
        pure
        returns (bytes4 functionSignature)
    {
        require(toSlice.length >= 4, "No function signature");

        assembly ("memory-safe") {
            functionSignature := mload(add(toSlice, 0x20))
        }
    }

    function test_check(bytes memory theBytes) public {
        vm.assume(theBytes.length >= 4);
        bytes4 res = getFunctionSignature(theBytes);
        bytes4 normal = bytes4(theBytes);

        assertEq(res, normal);
    }
LOW severity: Original Hot Signer that becomes Compromised or Malicious can take over all undeployed systems. **Impact**

The known issues specify:

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/docs/KNOWN_ISSUES.md#L5

This is correct, however, it's worth highlighting that malicious / compromised hot signers may be removed from a live deployment

However,...

Vulnerable code:
- if the hot signers are malicious or compromised, they can deploy a compromised system instance on a new chain with compromised recovery spells and malicious calldata checks that allow funds to be stolen
LOW severity: Docs Typos. **Merge marks**

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/docs/TESTING.md#L53-L59

**Pseudocode doesn't exist**

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/docs/CALLDATA_WHITELISTING.md#L34-L45

solidity...

Vulnerable code:
so that when there is a further sanity check failure by a mutant the spec can be marked as violated.
<<<<<<< HEAD
>>>>>>> 81590cd (remove redundant spec)
=======
>>>>>>> 0cf1c3c (remove redundant spec)
>>>>>>> 664c0e1 (remove redundant spec)
=======

This means that to supply to any market, the same function is called with different calldata parameters. This is where the array of calldata checks is used.

**Broken Link**

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/README.md#L7
HIGH severity: [G-01] `RecoverySpell` save gas by setting `recoveryInitiated = 0` to signify a Disabled Spell. **Gas - Set it to 0 to save gas due to refund**

https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/RecoverySpell.sol#L302

There is no zero timestamp so this is a safe change

Vulnerable code:
recoveryInitiated = type(uint256).max;
HIGH severity: [G-02] `ConfigurablePause.sol` - Save gas by using uint48. https://github.com/solidity-labs-io/kleidi/blob/1a06ac16bc99d0b4081281329d03064c3737f5e4/src/ConfigurablePause.sol#L15-L23

u48 is plenty and will allow one slot for all 3 values

Vulnerable code:
/// @notice pause start time, starts at 0 so contract is unpaused
    uint128 public pauseStartTime;

    /// @notice pause duration
    uint128 public pauseDuration;

    /// @notice address of the pause guardian
    address public pauseGuardian;
MEDIUM severity: Must accrue `stabilityPoolYieldSplit` before changing it. **Impact**

Changing the yield can change the results from `calcPendingSPYield`

https://github.com/subvisual/quill/blob/d4a5dcc168dfc315eef6a4c9c465a36c86ca0ddc/contracts/src/ActivePool.sol#L150-L153

This means that the estimated value could change

Based on integrations this can cause a...

Vulnerable code:
function calcPendingSPYield() external view returns (uint256) {
        return calcPendingAggInterest() * stabilityPoolYieldSplit / DECIMAL_PRECISION;
    }
MEDIUM severity: `CCR == SCR` can cause unintended shutdown when the protocol has only one Trove per Branch. **Impact**

The deployment scripts for Quill looks as follows:

https://github.com/subvisual/quill/blob/23e53123a16b12614d25bfb715e17dd41bcebbdd/contracts/scripts/DeployQuillLocal.s.sol#L38-L42

The MCR and SCR are matching, this means that for Branches that have one Trove, that trove could cause...

Vulnerable code:
TroveManagerParams[] memory troveManagerParamsArray = new TroveManagerParams[](4);
        troveManagerParamsArray[0] = TroveManagerParams(150e16, 110e16, 110e16, 5e16, 10e16, 500e18, 72e16, _1pct / 2); // WETH
        troveManagerParamsArray[1] = TroveManagerParams(150e16, 120e16, 110e16, 5e16, 10e16, 500e18, 72e16, _1pct / 2); // wstETH
        troveManagerParamsArray[2] = TroveManagerParams(150e16, 120e16, 110e16, 5e16, 10e16, 500e18, 72e16, _1pct / 2); // weETH
        troveManagerParamsArray[3] = TroveManagerParams(150e16, 120e16, 110e16, 5e16, 10e16, 500e18, 72e16, _1pct / 2); // SCROLL
MEDIUM severity: `QuillSimplePriceFeed` not having a `fetchRedemptionPrice` can open up to Oracle Drift Arbitrage via Redemptions. **Impact**

The pricing of a redemption in the `QuillSimplePriceFeed` is as follows:

https://github.com/subvisual/quill/blob/d4a5dcc168dfc315eef6a4c9c465a36c86ca0ddc/contracts/src/Quill/PriceFeeds/QuillSimplePriceFeed.sol#L28-L31

Meaning the price is the same as the one for borrowing

This is...

Vulnerable code:
function fetchRedemptionPrice() external returns (uint256, bool) {
        // Use same price for redemption as all other ops in WETH branch
        return fetchPrice(); /// @audit this may be underpriced due to Oracle Drift | You must raise the Redemption Fee to cover against it
    }