Farseen0/scar-pairs huggingface dataset:
MEDIUM severity: Stale Oracle prices accepted when no backup oracles available. **Description:** The `UpdateWeightRunner::getData` function performs staleness checks on oracle prices to ensure price data is fresh. However, when the optimized (primary) oracle returns stale data and there are no backup oracles configured, the function silently accepts and returns the stale...

Vulnerable code:
// getData function
function getData(address _pool) public returns (int256[] memory outputData) {
         // .... code
        outputData = new int256[](oracleLength);
        uint oracleStalenessThreshold = IQuantAMMWeightedPool(_pool).getOracleStalenessThreshold();

        for (uint i; i < oracleLength; ) {
            // Asset is base asset
            OracleData memory oracleResult;
            oracleResult = _getOracleData(OracleWrapper(optimisedOracles[i]));
            if (oracleResult.timestamp > block.timestamp - oracleStalenessThreshold) {
                outputData[i] = oracleResult.data; //@audit skips this correctly when data is stale
            } else {
                unchecked {
                    numAssetOracles = poolBackupOracles[_pool][i].length;
                }

                for (uint j = 1 /*0 already done via optimised poolOracles*/; j < numAssetOracles; ) { //@audit for loop is skipped when no backup oracle
                    oracleResult = _getOracleData(
                        // poolBackupOracles[_pool][asset][oracle]
                        OracleWrapper(poolBackupOracles[_pool][i][j])
                    );
                    if (oracleResult.timestamp > block.timestamp - oracleStalenessThreshold) {
                        // Oracle has fresh values
                        break;
                    } else if (j == numAssetOracles - 1) {
                        // All oracle results for this data point are stale. Should rarely happen in practice with proper backup oracles.

                        revert("No fresh oracle values available");
                    }
                    unchecked {
                        ++j;
                    }
                }
                outputData[i] = oracleResult.data; //@audit BUG - here it accepts the same stale data that was rejected before
            }

            unchecked {
                ++i;
            }
        }

        if(!internalCall){
            emit GetData(msg.sender, _pool);
        }
    }

else {
    unchecked {
        numAssetOracles = poolBackupOracles[_pool][i].length;
    }

    if (numAssetOracles == 1) {  // @audit revert when No backups available (1 accounts for primary oracle)
        revert("No fresh oracle values available");
    }

    for (uint j = 1; j < numAssetOracles; ) {
        // ... rest of the code
    }
}
HIGH severity: `QuantAMMBaseAdministration::onlyExecutor` modifier does not enforce a time lock on actions. **Description:** In `QuantAMMBaseAdministration`, certain functions are restricted using the [`onlyExecutor`](https://github.com/QuantAMMProtocol/QuantAMM-V1/blob/7213401491f6a8fd1fcc1cf4763b15b5da355f1c/pkg/pool-quantamm/contracts/QuantAMMBaseAdministration.sol#L77-L81) modifier:

The issue with...

Vulnerable code:
// Modifier to check for EXECUTOR_ROLE using `timelock`
modifier onlyExecutor() {
    require(timelock.hasRole(timelock.EXECUTOR_ROLE(), msg.sender), "Not an executor");
    _;
}

modifier onlyTimelock() {
       require(address(timelock) == msg.sender, "Only timelock");
       _;
   }

- TimelockController private timelock;
   + TimelockController public timelock;
HIGH severity: Unauthorized role grants prevent `QuantAMMBaseAdministration` deployment. **Description:** `QuantAMMBaseAdministration` is a contract that manages certain break-glass features and facilitates calls to other QuantAMM contracts.

However, this contract fails to deploy due to an issue in the [`QuantAMMBaseAdministration`...

Vulnerable code:
for (uint256 i = 0; i < proposers.length; i++) {
    timelock.grantRole(timelock.PROPOSER_ROLE(), proposers[i]);
}

for (uint256 i = 0; i < executors.length; i++) {
    timelock.grantRole(timelock.EXECUTOR_ROLE(), executors[i]);
}

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;

import "forge-std/Test.sol";
import "../../contracts/QuantAMMBaseAdministration.sol";

contract QuantAMMBaseAdministrationTest is Test {

    address daoRunner = makeAddr("daoRunner");

    address proposer = makeAddr("proposer");
    address executor = makeAddr("executor");

    QuantAMMBaseAdministration admin;

    function testSetupQuantAMMBaseAdministration() public {
        address[] memory proposers = new address[](1);
        proposers[0] = proposer;

        address[] memory executors = new address[](1);
        executors[0] = executor;

        vm.expectRevert(); // AccessControlUnauthorizedAccount(address(admin),DEFAULT_ADMIN_ROLE)
        admin = new QuantAMMBaseAdministration(
            daoRunner,
            1 hours,
            proposers,
            executors
        );
    }
}
LOW severity: Missing weight sum validation in `setWeightsManually` function. **Description:** The `UpdateWeightRunner::setWeightsManually` allows privileged users (DAO, pool manager, or admin based on pool registry) to manually set weights of the pool without validating if the weights sum to 1. This check is correctly enforced the first time when weights are initialized in.....

Vulnerable code:
// In QuantAMMWeightedPool.sol - _setInitialWeights
// Ensure that the normalized weights sum to ONE
if (uint256(normalizedSum) != FixedPoint.ONE) {
    revert NormalizedWeightInvariant();
}
LOW severity: Extreme weight ratios combined with large balances can cause denial-of-service for unbalanced liquidity operations. **Description:** The weighted pool math can potentially overflow when making unbalanced liquidity adjustments to a pool tokens with very small weights and large balances.

The issue occurs in `WeightedMath::computeBalanceOutGivenInvariant`:

When weights are small and invariant ratio is close to...

Vulnerable code:
newBalance = oldBalance * (invariantRatio ^ (1/weight))

// Balance computation scenario:
balance = 7500e21  (7.5 million tokens)
weight = 0.01166 (1.166%) // Just above absoluteWeightGuardRail minimum of 1% proposed for Balancer pools
invariantRatio = 3.0 // maximum value

calculation = 7500e21 * (3.0 ^ (1/0.01166))
           = 7500e21 * (3.0 ^ 85.76)
           = OVERFLOW

//applied both as a max (1 - x) and a min, so it cant be more than 0.49 or less than 0.01
//all pool logic assumes that absolute guard rail is already stored as an 18dp int256
require(
    int256(uint256(_poolSettings.absoluteWeightGuardRail)) <
        PRBMathSD59x18.fromInt(1) / int256(uint256((_initialWeights.length))) &&
        int256(uint256(_poolSettings.absoluteWeightGuardRail)) > 0.001e18, // @audit minimum guard rail allowed is 0.1 %
    "INV_ABSWGT"
); //Invalid absoluteWeightGuardRail value
HIGH severity: `AntimomentumUpdateRule.parameterDescriptions` initialized too long. **Description:** [`AntimomentumUpdateRule.parameterDescriptions`](https://github.com/QuantAMMProtocol/QuantAMM-V1/blob/7213401491f6a8fd1fcc1cf4763b15b5da355f1c/pkg/pool-quantamm/contracts/rules/AntimomentumUpdateRule.sol#L16-L18) is initialized as length 3 but only two entries are used:

Consider...

Vulnerable code:
parameterDescriptions = new string[](3); // @audit should be 2
parameterDescriptions[0] = "Kappa: Kappa dictates the aggressiveness of response to a signal change TODO";
parameterDescriptions[1] = "Use raw price: 0 = use moving average, 1 = use raw price to be used as the denominator";
HIGH severity: Unnecessary `_initialWeights` sum check. **Description:** When first initializing a `QuantAMMWeightedPool` the pool creator provides the initial weights. These initial weights should sum up to 1 (`1e18`). The issue is however that this is verified both in...

Vulnerable code:
int256 sumWeights;
for (uint i; i < _initialWeights.length; ) {
    sumWeights += int256(_initialWeights[i]);
    unchecked {
        ++i;
    }
}

require(
    sumWeights == 1e18, // 1 for int32 sum
    "SWGT!=1"
); //Initial weights must sum to 1

int256 normalizedSum;
int256[] memory _weightsAndBlockMultiplier = new int256[](_weights.length * 2);
for (uint i; i < _weights.length; ) {
    if (_weights[i] < int256(uint256(absoluteWeightGuardRail))) {
        revert MinWeight();
    }

    _weightsAndBlockMultiplier[i] = _weights[i];
    normalizedSum += _weights[i];
    //Initially register pool with no movement, first update will come and set block multiplier.
    _weightsAndBlockMultiplier[i + _weights.length] = int256(0);
    unchecked {
        ++i;
    }
}

// Ensure that the normalized weights sum to ONE
if (uint256(normalizedSum) != FixedPoint.ONE) {
    revert NormalizedWeightInvariant();
}
HIGH severity: SecuritizeVault's share token is in wrong decimals. **Description:** The `SecuritizeVault` inherits OpenZeppelin's `ERC4626Upgradeable` where share tokens maintain the same decimals as the underlying asset token by default. However, the vault overrides `_convertToShares()` and `_convertToAssets()` to integrate NAV provider rates, which are assumed...

Vulnerable code:
SecuritizeVault.sol
310:     function _convertToShares(uint256 _assets, Math.Rounding rounding) internal view virtual override(ERC4626Upgradeable) returns (uint256) {
311:         uint256 rate = navProvider.rate();//@audit-info D{liquidationToken}
312:         uint256 decimalsFactor = 10 ** decimals();
313:         uint256 totalSharesAfterDeposit = rate.mulDiv(totalAssets() + _assets, decimalsFactor, rounding);//@audit-info D{liquidationToken} * D{asset} / D{asset} -> D{liquidationToken}
314:         if (totalSharesAfterDeposit >= totalSupply()) {
315:             return totalSharesAfterDeposit - totalSupply();//@audit-issue the return amount must be in the decimals of the share token but it is in liquidationToken's decimals
316:         }
317:         return 0;
318:     }

333:     function _convertToAssets(uint256 _shares, Math.Rounding rounding) internal view virtual override(ERC4626Upgradeable) returns (uint256) {
334:         if (totalSupply() == 0) {
335:             return 0;
336:         }
337:         uint256 rate = navProvider.rate();//@audit-info D{liquidationToken}
338:         uint256 decimalsFactor = 10 ** decimals();
339:         return Math.min(_shares.mulDiv(decimalsFactor, rate, rounding), _shares.mulDiv(totalAssets(), totalSupply(), rounding));//@audit-info D{share} * D{asset} / D{liquidationToken}, D{share} * D{asset} / D{share}
340:     }

function testShareDecimalsWrong() public {
        navProvider.setRate(1e6);// nav provider rate is 1 in the decimals of liquidationToken

        // deposit
        vm.startPrank(owner);
        uint256 depositAmount = 1000e18;// 1000 asset tokens
        assetToken.mint(owner, depositAmount);
        assetToken.approve(address(vault), depositAmount);
        vault.deposit(depositAmount, owner);
        uint256 redeemShares = vault.balanceOf(owner); // 100% redeem
        emit log_named_uint("Share token decimals", vault.decimals()); // 18
        emit log_named_uint("Share amount", redeemShares); // 1000 * 1e6

        vm.stopPrank();
    }

forge test -vvv --match-test testShareDecimalsWrong

Ran 1 test for test/forge/SecuritizeVaultTest_R.t.sol:SecuritizeVaultTest_R
[PASS] testShareDecimalsWrong() (gas: 159913)
Logs:
  Share token decimals: 18
  Share amount: 1000000000
MEDIUM severity: Lack of slippage protection in the SecuritizeVault's liquidation function. **Description:** The SecuritizeVault's `liquidate()` function converts share tokens to asset tokens using the NAV provider's rate without allowing users to specify a minimum output amount. Since the conversion rate is dynamic and external redemption may be involved, users are exposed to potential...

Vulnerable code:
function liquidate(uint256 shares, uint256 minOutputAmount) public override(ISecuritizeVault) whenNotPaused {
...
    require(assets >= minOutputAmount, "Insufficient output amount");
...
}
MEDIUM severity: Inconsistent token amount in external redemption process of SecuritizeVault's liquidate function. **Description:** In SecuritizeVault's liquidation process with external redemption, there's likely a mismatch between the actual received stable coins from the redemption contract and the amount calculated to send to the liquidator. The vault calculates the output amount using `navProvider.rate()`.....

Vulnerable code:
SecuritizeVault.sol
261:         if (address(0) != address(redemption)) {
262:             IERC20(asset()).approve(address(redemption), assets);
263:             redemption.redeem(assets);//@audit-info this sends underlying token to the redemption, receives stablecoin into this contract
264:             uint256 rate = navProvider.rate();
265:             uint256 decimalsFactor = 10 ** decimals();
266:             // after external redemption, vault gets liquidity to supply msg.sender (assets * nav)
267:             // liquidationToken === stableCoin
268:             liquidationToken.safeTransfer(msg.sender, assets.mulDiv(rate, decimalsFactor, Math.Rounding.Floor));//@audit-issue possible inconsistency in the amoutns. consider sending the balance delta instead
269:         }

if (address(0) != address(redemption)) {
        IERC20 liquidityToken = IERC20(redemption.liquidity());
        uint256 balanceBefore = liquidityToken.balanceOf(address(this));
        redemption.redeem(assets);
        uint256 receivedAmount = liquidityToken.balanceOf(address(this)) - balanceBefore;
        liquidityToken.transfer(msg.sender, receivedAmount);
    }
MEDIUM severity: Ambiguous owner terminology creates confusion in access control of SecuritizeVault. **Description:** SecuritizeVault implements two different ownership concepts that create confusion in access control. It uses a custom `OWNER_ROLE` for vault operations (deposit/redeem) while inheriting `OwnableUpgradeable` which provides its own `owner()` function. This creates a situation where...

Vulnerable code:
function testPause() public {
    // admin (the deployer) is Ownable owner - can pause/unpause
    vm.startPrank(admin);
    vault.pause();
    assertTrue(vault.paused());
    vault.unpause();
    vm.stopPrank();

    // OWNER_ROLE holder cannot pause
    vm.startPrank(owner);
    vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, owner));
    vault.pause();
    vm.stopPrank();

    // Demonstrating the confusion:
    assertTrue(vault.isOwner(owner));     // True: has OWNER_ROLE
    assertFalse(vault.isOwner(admin));    // False: doesn't have OWNER_ROLE
    assertTrue(vault.owner() == admin);    // True: is Ownable owner
}

// Instead of OWNER_ROLE
bytes32 public constant VAULT_OPERATOR_ROLE = keccak256("VAULT_OPERATOR_ROLE");

// Instead of isOwner()
function isVaultOperator(address account) public view returns (bool) {
    return hasRole(VAULT_OPERATOR_ROLE, account);
}
HIGH severity: Missing input validation in privileged functions. **Description:** Admin functions lack proper parameter validation, which could lead to unintended state changes if incorrect values are provided. While admin users are expected to act correctly, human error remains possible.

**Securitize:** Fixed in commit...

Vulnerable code:
SecuritizeRedemption.sol
75:     function initialize(address _asset, address _navProvider) public onlyProxy initializer navProviderNonZero(_navProvider) {
76:         __BaseDSContract_init();
77:         asset = IERC20(_asset);//@audit-issue INFO non zero check
78:         navProvider = ISecuritizeNavProvider(_navProvider);
79:     }
80:

SecuritizeRedemption.sol
81:     function updateLiquidityProvider(address _liquidityProvider) onlyOwner external {
82:         address oldProvider = address(liquidityProvider);
83:         liquidityProvider = ILiquidityProvider(_liquidityProvider);//@audit-issue INFO non zero check
84:         emit LiquidityProviderUpdated(oldProvider, address(liquidityProvider));
85:     }

SecuritizeRedemption.sol
87:     function updateNavProvider(address _navProvider) onlyOwner navProviderNonZero(_navProvider) external {
88:         address oldProvider = address(navProvider);
89:         navProvider = ISecuritizeNavProvider(_navProvider);//@audit-issue INFO non zero check
90:         emit NavProviderUpdated(oldProvider, address(navProvider));
91:     }

CollateralLiquidityProvider.sol
66:     function initialize(address _recipient, address _liquidityToken, address _securitizeRedemption) public onlyProxy initializer {
67:         __BaseDSContract_init();
68:         recipient = _recipient;//@audit-issue LOW sanity check
69:         liquidityToken = IERC20(_liquidityToken);
70:         securitizeRedemption = ISecuritizeRedemption(_securitizeRedemption);
71:     }

98:
99:     function setCollateralProvider(address _collateralProvider) external onlyOwner {
100:         address oldAddress = address(collateralProvider);
101:         collateralProvider = _collateralProvider;//@audit-issue LOW sanity check
102:         emit CollateralProviderUpdated(oldAddress, address(collateralProvider));
103:     }

AllowanceLiquidityProvider.sol
65:     function initialize(address _recipient, address _liquidityToken, address _securitizeRedemption) public onlyProxy initializer {
66:         __BaseDSContract_init();
67:         recipient = _recipient;//@audit-issue LOW sanity check
68:         liquidityToken = IERC20(_liquidityToken);
69:         securitizeRedemption = ISecuritizeRedemption(_securitizeRedemption);
70:     }

85:     function setAllowanceProviderWallet(address _liquidityProviderWallet) external onlyOwner {
86:         address oldAddress = liquidityProviderWallet;
87:         liquidityProviderWallet = _liquidityProviderWallet;//@audit-issue sanity check
88:         emit AllowanceLiquidityProviderWalletUpdated(oldAddress, liquidityProviderWallet);
89:     }
LOW severity: Insufficient storage gap in BaseSecuritizeSwap contract. **Description:** The BaseSecuritizeSwap contract implements a storage gap of 40 slots (`uint256[40] __gap`), which doesn't follow OpenZeppelin's practice of reserving 50 storage slots in total for upgradeable contracts. This deviation from the standard could potentially cause storage collision...

Vulnerable code:
abstract contract BaseSecuritizeSwap is BaseDSContract, PausableUpgradeable {
    // ... other contract code ...
    uint256[40] __gap; // @audit-issue LOW -> uint256[43]
}

forge inspect BaseSecuritizeSwap storage --pretty
| Name                         | Type                             | Slot | Offset | Bytes | Contract                                                 |
|------------------------------|----------------------------------|------|--------|-------|----------------------------------------------------------|
| services                     | mapping(uint256 => address)      | 0    | 0      | 32    | BaseSecuritizeSwap |
| __gap                        | uint256[49]                      | 1    | 0      | 1568  | BaseSecuritizeSwap |
| dsToken                      | contract IDSToken                | 50   | 0      | 20    | BaseSecuritizeSwap |
| stableCoinToken              | contract IERC20                  | 51   | 0      | 20    | BaseSecuritizeSwap |
| erc20Token                   | contract IERC20                  | 52   | 0      | 20    | BaseSecuritizeSwap |
| collateralToken              | contract IERC20                  | 53   | 0      | 20    | BaseSecuritizeSwap |
| issuerWallet                 | address                          | 54   | 0      | 20    | BaseSecuritizeSwap |
| liquidityProviderWallet      | address                          | 55   | 0      | 20    | BaseSecuritizeSwap |
| externalCollateralRedemption | contract IRedemption             | 56   | 0      | 20    | BaseSecuritizeSwap |
| swapMode                     | enum BaseSecuritizeSwap.SwapMode | 56   | 20     | 1     | BaseSecuritizeSwap |
| __gap                        | uint256[40]                      | 57   | 0      | 1280  | BaseSecuritizeSwap |

abstract contract BaseSecuritizeSwap is BaseDSContract, PausableUpgradeable {
--    uint256[40] __gap;
++    uint256[43] __gap;
}
LOW severity: Missing PausableUpgradable initialization in SecuritizeSwap contract initialization. **Description:** The `SecuritizeSwap` contract inherits from `PausableUpgradeable` (through `BaseSecuritizeSwap`) but doesn't call `__Pausable_init()` during initialization. While this currently doesn't affect functionality since the default state (paused = false) matches the initialization state,.....

Vulnerable code:
function initialize(...) public override initializer onlyProxy {
    BaseSecuritizeSwap.initialize(
        _dsToken,
        _stableCoin,
        _erc20Token,
        _issuerWallet,
        _liquidityProvider,
        _externalCollateralRedemption,
        _collateralToken,
        _swapMode
    );
    __BaseDSContract_init();
    __Pausable_init();
}
LOW severity: Potential replay attacks due to static DOMAIN_SEPARATOR. **Description:** The DOMAIN_SEPARATOR is set only once during initialization and includes `block.chainid`. This could theoretically enable cross-chain replay attacks in the event of a hard fork.
Although the contract implements a nonce system (`mapping(string => uint256) internal...

Vulnerable code:
function initialize(...) public override initializer onlyProxy {
    // ... other initialization code ...

    DOMAIN_SEPARATOR = keccak256(
        abi.encode(
            EIP712_DOMAIN_TYPE_HASH,
            NAME_HASH,
            VERSION_HASH,
            block.chainid,  //@audit-issue LOW this can change in the case of hard fork
            this,
            SALT
        )
    );
}

function DOMAIN_SEPARATOR() public view returns (bytes32) {
    return keccak256(
        abi.encode(
            EIP712_DOMAIN_TYPE_HASH,
            NAME_HASH,
            VERSION_HASH,
            block.chainid,
            this,
            SALT
        )
    );
}
HIGH severity: Share tokens can be transferred. **Description:** The `SecuritizeVault` implements ERC4626's `deposit` function with an additional restriction requiring the depositor to be the same as the receiver (`_msgSender() == receiver`). While this was intended to ensure vault tokens are only owned by the designated investor, this...

Vulnerable code:
SecuritizeVault.sol
205:     function deposit(uint256 assets, address receiver)
206:         public
207:         override(ERC4626Upgradeable, ISecuritizeVault)
208:         whenNotPaused
209:         onlyRole(OWNER_ROLE)
210:         returns (uint256)
211:     {
212:         require(_msgSender() == receiver, "Sender should be equal than receiver");//@audit-issue not meaningful because share tokens can be transferred
213:         return super.deposit(assets, receiver);
214:     }
215: