Farseen0/scar-pairs huggingface dataset:
LOW severity: `PolygonStrategy::upgradeVaults` does not verify if vaults to upgrade are the same as the ones stored in strategy. **Description:** `PolygonStrategy::upgradeVaults` takes an input of `_vaults` array and upgrades them to the latest `vaultImplementation` as follows:

Current implementation has several issues:

1. There is no check to verify if the input addresses actually match the ones locally stored in the...

Vulnerable code:
function upgradeVaults(address[] calldata _vaults, bytes[] memory _data) external onlyOwner {

        for (uint256 i = 0; i < _vaults.length; ++i) { //@audit no check if the _vault address is actually one locally stored
            if (_data.length == 0 || _data[i].length == 0) {
                IPolygonVault(_vaults[i]).upgradeTo(vaultImplementation);
            } else {
                IPolygonVault(_vaults[i]).upgradeToAndCall(vaultImplementation, _data[i]);
            }
        }
        emit UpgradedVaults(); //@audit does not emit the list of vaults actually updated
    }
LOW severity: Reward restaking creates unnecessary unbonding in edge cases. **Description:** The `PolygonStrategy::restakeRewards()` function is publicly callable by anyone, allowing rewards to be restaked immediately before the `PolygonStrategy::unbond()` function is called.

In specific scenarios, this can cause the protocol to unnecessarily unbond principal funds when...

Vulnerable code:
// In PolygonStrategy::unbond()
uint256 deposits = vault.getTotalDeposits();
uint256 principalDeposits = vault.getPrincipalDeposits();
uint256 rewards = deposits - principalDeposits; //@audit this would be 0 if restakeRewards is called just before

if (rewards >= toUnbondRemaining) {
    vault.withdrawRewards();
    toUnbondRemaining = 0;
} else {
    toUnbondRemaining -= rewards;
    uint256 vaultToUnbond = principalDeposits >= toUnbondRemaining
        ? toUnbondRemaining
        : principalDeposits;

    vault.unbond(vaultToUnbond);

    toUnbondRemaining -= vaultToUnbond;
    ++numVaultsUnbonded; //@audit this will be 1 more than necessary
}

// In PolygonStrategy::restakeRewards()
function restakeRewards(uint256[] calldata _vaultIds) external {
    for (uint256 i = 0; i < _vaultIds.length; ++i) {
        vaults[_vaultIds[i]].restakeRewards();
    }

    emit RestakeRewards();
}

describe.only('restakeRewards forces protocol to wait for withdrawal delay', async () => {
    it('should force protocol to wait for withdrawal delay', async () => {
      const { token, strategy, vaults, fundFlowController, withdrawalPool, validatorShare } =
        await loadFixture(deployFixture)

      await withdrawalPool.setTotalQueuedWithdrawals(toEther(960))
      assert.equal(await fundFlowController.shouldUnbondVaults(), true);

      // 1. Pre-condition: one validator can cover the amount to unbond with his rewards.
      await validatorShare.addReward(vaults[0].target, toEther(1000));

      // 2. User front-runs the unbondVaults transaction and restakes rewards for validators.
      await strategy.restakeRewards([0]);

      const totalQueuedBefore = await strategy.totalQueued();
      // 3. Protocol is forced to unbond and wait for withdrawal delay even though
      // it could obtain the funds after calling vault.withdrawRewards.
      await fundFlowController.unbondVaults()
      const totalQueuedAfter = await strategy.totalQueued();

      assert.equal(await fundFlowController.shouldUnbondVaults(), false)

      // 4. TotalQueued did not increase, meaning strategy could not withdraw any funds
      // and vault is unbending(it wouldn't be if it cover the funds with the rewards).
      assert.equal(totalQueuedBefore, totalQueuedAfter)
      assert.equal(await vaults[0].isUnbonding(), true)
    })
  })
HIGH severity: DoS in unbonding when validator rewards fall below min amount threshold. **Description:** The [unbond](https://github.com/stakedotlink/contracts/blob/1d3aa1ed6c2fb7920b3dc3d87ece5d1645e1a628/contracts/polygonStaking/PolygonStrategy.sol#L213-L263) function in `PolygonStrategy` is vulnerable to DoS when small reward amounts are used to cover the remaining amount to...

Vulnerable code:
function unbondVaults() external {
       ...
        uint256 toWithdraw = queuedWithdrawals - (queuedDeposits + validatorRemovalDeposits);
@>        strategy.unbond(toWithdraw);
        timeOfLastUnbond = uint64(block.timestamp);
    }

function unbond(uint256 _toUnbond) external onlyFundFlowController {
   ...
    if (rewards >= toUnbondRemaining) {
        // @audit withdrawRewards could be below the threshold from ValidatorShares
@>        vault.withdrawRewards();
        toUnbondRemaining = 0;
    } else {
        toUnbondRemaining -= rewards;
        uint256 vaultToUnbond = principalDeposits >= toUnbondRemaining
            ? toUnbondRemaining
            : principalDeposits;
@>        vault.unbond(vaultToUnbond);
        toUnbondRemaining -= vaultToUnbond;
        ++numVaultsUnbonded;
    }
   ...
}

uint256 deposits = vault.getTotalDeposits();
uint256 principalDeposits = vault.getPrincipalDeposits();
uint256 rewards = deposits - principalDeposits;
HIGH severity: Precision Manipulation Due to Missing Mint Check. **Description:** `deposit_handler` chooses the conversion precision (`conv_decimals`) as:

The account context *documents* that `liquidation_token_mint` and `liquidation_token_vault` must appear together, but there is **no runtime constraint** enforcing it.
An operator can therefore:

1. Pass...

Vulnerable code:
let conv_decimals = ctx
    .accounts
    .liquidation_token_mint
    .as_ref()
    .map(|mint| mint.decimals)          // if Some ⇒ use its decimals
    .unwrap_or(ctx.accounts.asset_mint.decimals);

require!(
      liquidation_token_mint.is_some() == liquidation_token_vault.is_some(),
      ScVaultError::InvalidLiquidationAccounts
  );
MEDIUM severity: Missing Slippage Check on `liquidation_amount` in Redemption-Enabled Vaults. **Description:** When calling `liquidate_handler()` the liquidator provide the minimum amount of assets he is willing to receive. either assets or liquidation tokens

The slippage check is implemented only for assets before checking the type of the Vault weather it support Redemption or not.

As...

Vulnerable code:
/// ## Arguments
...
/// - `min_output_amount`: An optional minimum output amount to ensure sufficient assets or liquidation tokens are received.

>>  if let Some(min_output_amount) = min_output_amount {
        require_gt!(
            assets,
            min_output_amount,
            ScVaultError::InsufficientOutputAmount
        );
    }

    ...

    if let Some(ref redemption_program) = ctx.accounts.redemption_program {
        ...
        // Transfer received liquidation tokens to liquidator.
        transfer_from!(
            liquidation_token_vault,                                  // from
            liquidator_liquidation_ata,                        // to
            ctx.accounts.vault_authority,                             // authority
            ctx.accounts.liquidation_token_program.as_ref().unwrap(), // token_program
            liquidation_token_mint,                                   // mint
>>          liquidation_amount,                                       // amount
            liquidation_token_mint.decimals,                          // decimals
            vault_authority_signer                                    // signer
        );
    } else {
        ...
    }
LOW severity: Off-By-One Error in Liquidator and Operator Capacity Check. **Description:** When adding new liquidators or operators, the check is performed to check that the MAX_LENGTH is greater than or equal the current length. But after this we push the new element.

> - bc-solana-vault-sc/programs/sc-vault/src/instructions/admin/add_liquidator.rs
>-...

Vulnerable code:
pub fn add_liquidator_handler(
    ctx: &mut Context<AddLiquidator>,
    new_liquidator: Pubkey,
) -> Result<()> {
    let vault_state = &mut ctx.accounts.vault_state;

>>  require_gte!(
        MAX_LIQUIDATORS,
        vault_state.liquidators.len(),
        ScVaultError::MaxLiquidators
    );
    ...
>>  vault_state.liquidators.push(new_liquidator);
    ...
}
// ----------------
pub fn add_operator_handler(ctx: &mut Context<AddOperator>, new_operator: Pubkey) -> Result<()> {
    let vault_state = &mut ctx.accounts.vault_state;

>>  require_gte!(
        MAX_OPERATORS,
        vault_state.operators.len(),
        ScVaultError::MaxOperators
    );
    ...
>>  vault_state.operators.push(new_operator);
    ...
}
LOW severity: Unsafe Addition in `convert_to_shares` Causes View Function Inaccuracy. **Description:** When providing `assets` parameter there is no check if the addition of `total_assets` with `asset` will result in a number greater than `u64::MAX` or not.

When calling `convert_to_assets_handler`, the asset is provided as input. it is like a `view` function to check the...

Vulnerable code:
pub fn convert_to_shares(
    assets: u64,
    rate: u64,
    decimals: u8,
    total_assets: u64,
    total_supply: u64,
    rounding: Rounding,
) -> Result<u64> {
    let liq_token_decimals_factor = 10u64.pow(decimals.into());
    let total_shares_after_deposit = math::mul_div(
        rate,
>>      total_assets + assets,
        liq_token_decimals_factor,
        rounding,
    )?;
    ...
}
LOW severity: Strict Comparison in Slippage Check Incorrectly Blocks Valid Redemptions. **Description:** When checking minimum outcome from the liquidate, we are implementing the check to enforce the returned assets to be more than the minimum amount we put.

> bc-solana-vault-sc/programs/sc-vault/src/instructions/liquidator/liquidate.rs#liquidate_handler

This check is incorrect as...

Vulnerable code:
if let Some(min_output_amount) = min_output_amount {
>>      require_gt!(
            assets,
            min_output_amount,
            ScVaultError::InsufficientOutputAmount
        );
    }
LOW severity: Incorrect Splitting of `remaining_accounts` Causes Misrouting Between NAV and Redemption Accounts. **Description:** When spliting accounts in `liquidate_handler`, we are assuming that `nav_provider_program` will take the MAX value. where we take  the minimum value from the `MAX_NAV_PROVIDER_ACCOUNTS (5)` and the remaining accounts.

>...

Vulnerable code:
let nav_provider_accounts_count = MAX_NAV_PROVIDER_ACCOUNTS.min(ctx.remaining_accounts.len());
    let (nav_provider_accounts, redemption_accounts) =
        ctx.remaining_accounts.split_at(nav_provider_accounts_count);
LOW severity: `Liquidate` Event Emits Shares and Assets in Wrong Order. **Description:** The `Liquidate` event is fired making `shares` as the second parameter and `assets` as the third parameter

> bc-solana-vault-sc/programs/sc-vault/src/instructions/liquidator/liquidate.rs#liquidate_handler

But the event construction is not like this, as `assets` are the second...

Vulnerable code:
emit!(crate::events::Liquidate {
        liquidator: ctx.accounts.liquidator.key(),
2:      shares,
3:      assets,
    });

#[event]
pub struct Liquidate {
    pub liquidator: Pubkey,
2:  pub assets: u64,
3:  pub shares: u64,
}

emit!(crate::events::Liquidate {
        liquidator: ctx.accounts.liquidator.key(),
-       shares,
        assets,
+       shares,
    });
HIGH severity: Protocol Fee Recipient Updates Are Not Enforced On Old EulerSwap Instances. **Description:** The `protocolFeeRecipient` value is stored in the parameters of each `EulerSwap` instance at the time of its deployment. When the [`ProtocolFee::setProtocolFeeRecipient`](https://github.com/euler-xyz/euler-swap/blob/1022c0bb3c034d905005f4c5aee0932a66adf4f8/src/utils/ProtocolFee.sol#...

Vulnerable code:
function test_protocolFeeRecipientChange() public {
	// Define two protocol fee recipients
	address recipient1 = makeAddr("recipient1");
	address recipient2 = makeAddr("recipient2");

	// Set initial protocol fee and recipient in factory
	uint256 protocolFee = 0.1e18; // 10% of LP fee
	eulerSwapFactory.setProtocolFee(protocolFee);
	eulerSwapFactory.setProtocolFeeRecipient(recipient1);

	// Deploy pool1 with recipient1
	EulerSwap pool1 = createEulerSwapHookFull(
		60e18,
		60e18,
		0.001e18,
		1e18,
		1e18,
		0.4e18,
		0.85e18,
		protocolFee,
		recipient1
	);

	// Verify pool1's protocolFeeRecipient
	IEulerSwap.Params memory params1 = pool1.getParams();
	assertEq(params1.protocolFeeRecipient, recipient1);

	// Perform a swap in pool1
	uint256 amountIn = 1e18;
	assetTST.mint(anyone, amountIn);
	vm.startPrank(anyone);
	assetTST.approve(address(minimalRouter), amountIn);
	bool zeroForOne = address(assetTST) < address(assetTST2);
	minimalRouter.swap(pool1.poolKey(), zeroForOne, amountIn, 0, "");
	vm.stopPrank();

	// Check that fees were sent to recipient1
	uint256 feeCollected1 = assetTST.balanceOf(recipient1);
	assertGt(feeCollected1, 0);
	assertEq(assetTST.balanceOf(recipient2), 0);

	// Change protocolFeeRecipient to recipient2 in factory
	eulerSwapFactory.setProtocolFeeRecipient(recipient2);

	// Perform another swap in pool1
	assetTST.mint(anyone, amountIn);
	vm.startPrank(anyone);
	assetTST.approve(address(minimalRouter), amountIn);
	minimalRouter.swap(pool1.poolKey(), zeroForOne, amountIn, 0, "");
	vm.stopPrank();

	// Check that additional fees were sent to recipient1, not recipient2
	uint256 newFeeCollected1 = assetTST.balanceOf(recipient1);
	assertGt(newFeeCollected1, feeCollected1); // recipient1 received more fees
	assertEq(assetTST.balanceOf(recipient2), 0); // recipient2's balance unchanged
}
HIGH severity: Lack of input validation. **Description:** In the [constructor](https://github.com/euler-xyz/euler-swap/blob/1022c0bb3c034d905005f4c5aee0932a66adf4f8/src/EulerSwapFactory.sol#L44-L50) for `EulerSwapFactory` there's no sanity check that the addresses for  `evkFactory_`, `eulerSwapImpl_`, and `feeOwner_` aren't `address(0)`...

Vulnerable code:
require(evkFactory_ != address(0), "Zero address");
require(eulerSwapImpl_ != address(0), "Zero address");
require(feeOwner_ != address(0), "Zero address");
HIGH severity: Unnecessary extra storage reads when swapping. **Description:** When a swap is executed, whether via EulerSwap or Uniswap V4, a reentrant hook is employed:

[`UniswapHook::nonReentrantHook`](https://github.com/euler-xyz/euler-swap/blob/1022c0bb3c034d905005f4c5aee0932a66adf4f8/src/UniswapHook.sol#L67-L80):

Here, `CtxLib.getStorage()` is called.....

Vulnerable code:
modifier nonReentrantHook() {
    {
        CtxLib.Storage storage s = CtxLib.getStorage();
        require(s.status == 1, LockedHook());
        s.status = 2;
    }

    _;

    {
        CtxLib.Storage storage s = CtxLib.getStorage();
        s.status = 1;
    }
}

function calcLimits(IEulerSwap.Params memory p, bool asset0IsInput) internal view returns (uint256, uint256) {
       CtxLib.Storage storage s = CtxLib.getStorage();
       …
   }

function findCurvePoint(IEulerSwap.Params memory p, uint256 amount, bool exactIn, bool asset0IsInput)
       internal
       view
       returns (uint256 output)
   {
       CtxLib.Storage storage s = CtxLib.getStorage();
       …
   }
HIGH severity: Vault calls can be done directly to EVC. **Description:** In [`FundsLib::depositAssets`](https://github.com/euler-xyz/euler-swap/blob/1022c0bb3c034d905005f4c5aee0932a66adf4f8/src/libraries/FundsLib.sol#L61-L114), two calls are made to the Euler Vault:

* [Line...

Vulnerable code:
uint256 repaid = IEVault(vault).repay(amount > debt ? debt : amount, p.eulerAccount);

try IEVault(vault).deposit(amount, p.eulerAccount) {}

function repay(uint256 amount, address receiver) public virtual override callThroughEVC use(MODULE_BORROWING) returns (uint256) {}
HIGH severity: `isNewYToken` can be omitted in YToken contracts. **Description:** To support the accounting of underlying assets, a new parameter `isNewYToken` was introduced in `mintYToken`. This parameter is used in the [`dYTokenL1::mintYToken`](https://github.com/YieldFiLabs/contracts/blob/702a931df3adb2f6e48807203cdc7a92604ea249/contracts/core/tokens/dYTokenL...

Vulnerable code:
function mintYToken(address to, uint256 shares, bool isNewYToken) external virtual {
    require(msg.sender == manager, "!manager");
    _mint(to, shares);
}

- function mintYToken(address to, uint256 shares, bool isNewYToken) external virtual {
+ function mintYToken(address to, uint256 shares, bool ) external virtual {
HIGH severity: Avoid unnecessary computation in `dYToken::mintYToken` when `isNewYToken == false`. **Description:** In the new [`dYToken::mintYToken`](https://github.com/YieldFiLabs/contracts/blob/702a931df3adb2f6e48807203cdc7a92604ea249/contracts/core/tokens/dYTokenL1.sol#L67-L81), there is special logic for handling newly minted `dYTokens`, i.e., tokens generated through deposits or accrued...

Vulnerable code:
function mintYToken(address to, uint256 shares, bool isNewYToken) external override {
    require(msg.sender == manager, "!manager");
    uint256 assets = convertToAssets(shares);

    // if isNewYToken i.e external deposit has triggered minting of dyToken, we mint yToken to this contract
    if(isNewYToken) {
        // corresponding shares of yToken based on assets
        uint256 yShares = YToken(yToken).convertToShares(assets);
        // can pass isNewYToken here as it is not used in yToken
        ManageAssetAndShares memory manageAssetAndShares = ManageAssetAndShares({
            yToken: yToken,
            shares: yShares,
            assetAmount: assets,
            updateAsset: true,
            isMint: true,
            isNewYToken: isNewYToken
        });
        IManager(manager).manageAssetAndShares(address(this), manageAssetAndShares);
    }
    // minting dYToken to receiver
    _mint(to, shares);
}

function mintYToken(address to, uint256 shares, bool isNewYToken) external override {
    require(msg.sender == manager, "!manager");
-   uint256 assets = convertToAssets(shares);

    // if isNewYToken i.e external deposit has triggered minting of dyToken, we mint yToken to this contract
    if(isNewYToken) {
+       uint256 assets = convertToAssets(shares);
        // corresponding shares of yToken based on assets
        uint256 yShares = YToken(yToken).convertToShares(assets);
LOW severity: Missing common Chainlink Oracle validations. **Description:** The protocol is missing common [Chainlink Oracle validations](https://medium.com/contractlevel/chainlink-oracle-defi-attacks-93b6cb6541bf); it calls `AggregatorV3Interface::latestRoundData` without any validation of the result:

**Recommended Mitigation:** Implement common...

Vulnerable code:
function _getLatestPrice() internal view returns (uint256) {
    //slither-disable-next-line unused-return
    (, int256 price,,,) = i_nativeUsdFeed.latestRoundData();
    return uint256(price);
}
LOW severity: Users who are removed from the blacklist have to pay again for their NFT. **Description:** When a user is added to the blacklist, the NFT which they already paid for is burned:

But when they are removed from the blacklist, they do not receive a free NFT to make up for their previously burned one, nor is there any flag set that would enable them to mind their NFT again...

Vulnerable code:
function _addToBlacklist(address account) internal {
    // *snip: code not relevant *//

    if (balanceOf(account) > 0) {
        uint256 tokenId = tokenOfOwnerByIndex(account, 0); // Get first token
        _burn(tokenId); // Burn the token
    }
}

function _removeFromBlacklist(address account) internal {
    if (!s_blacklist[account]) revert SoulBoundToken__NotBlacklisted(account);

    s_blacklist[account] = false;
    emit RemovedFromBlacklist(account);
}
LOW severity: Round up fee against users. **Description:** Solidity by default rounds down, but generally fees should be rounded up against users. Using Solady's [library](https://github.com/Vectorized/solady/blob/main/src/utils/FixedPointMathLib.sol) is significantly more efficient than OpenZeppelin:

A secondary benefit of using the...

Vulnerable code:
import {FixedPointMathLib} from "@solady/utils/FixedPointMathLib.sol";

function _getFee() internal view returns (uint256 fee) {
    // read fee factor directly to output variable
    fee = s_feeFactor;

    // only do extra work if non-zero
    if(fee != 0) fee = FixedPointMathLib.fullMulDivUp(fee, PRICE_FEED_PRECISION, _getLatestPrice());
}

function _getFee() internal view returns (uint256 fee) {
    // read fee factor directly to output variable
    fee = s_feeFactor;

    // only do extra work if non-zero
    if(fee != 0) fee = (fee * PRICE_FEED_PRECISION) / _getLatestPrice();
}
HIGH severity: `_verifySignature` is not compatible with smart contract wallets or other smart accounts. **Description:** Support for smart accounts (e.g. [ERC-4337](https://eips.ethereum.org/EIPS/eip-4337)) and other smart contract wallets (e.g. Safe{Wallet}) minting tokens is not currently possible as the signature verification implemented in `_verifySignature` is only able to handle those...

Vulnerable code:
function _verifySignature(bytes memory signature) internal view returns (bool) {
        /// @dev compute the message hash: keccak256(termsHash, msg.sender)
        bytes32 messageHash = keccak256(abi.encodePacked(s_termsHash, msg.sender));

        /// @dev apply Ethereum signed message prefix
        bytes32 ethSignedMessageHash = MessageHashUtils.toEthSignedMessageHash(messageHash);

        /// @dev attempt to recover the signer
        //slither-disable-next-line unused-return
        (address recovered, ECDSA.RecoverError error,) = ECDSA.tryRecover(ethSignedMessageHash, signature);

        /// @dev return false if errors or incorrect signer
++      if (error == ECDSA.RecoverError.NoError && recovered == msg.sender) return true;
++      else return SignatureChecker.isValidERC1271SignatureNow(msg.sender, ethSignedMessageHash, signature);
    }
LOW severity: Use solady `safeTransferETH` to send eth. **Description:** Using solady [`safeTransferETH`](https://github.com/Vectorized/solady/blob/main/src/utils/SafeTransferLib.sol#L90-L98) is a [more efficient](https://github.com/devdacian/solidity-gas-optimization?tab=readme-ov-file#10-use-safetransferlibsafetransfereth-instead-of-solidity-call-effec...

Vulnerable code:
function withdrawFees() external onlyOwner {
    uint256 amountToWithdraw = address(this).balance;
    if(amountToWithdraw > 0) {
        // from https://github.com/Vectorized/solady/blob/main/src/utils/SafeTransferLib.sol#L90-L98
        /// @solidity memory-safe-assembly
        assembly {
            if iszero(call(gas(), caller(), amountToWithdraw, codesize(), 0x00, codesize(), 0x00)) {
                mstore(0x00, 0xefde920d) // `SoulBoundToken__WithdrawFailed()`.
                revert(0x1c, 0x04)
            }
        }

        emit FeesWithdrawn(amountToWithdraw);
    }
}

{
- "withdrawFees": "13462"
+ "withdrawFees": "13353"
}
LOW severity: Minimum deposit value not enforced in `LINKMigrator`. **Description:** The `LINKMigrator` contract includes a `queueDepositMin` field intended to define the minimum deposit amount. However, this value is currently unused, allowing users to deposit amounts as small as 1 juel (1e-18 LINK).

**Impact:** Without enforcement, users can submit deposits...

Vulnerable code:
function initiateMigration(uint256 _amount) external {
-     if (_amount == 0) revert InvalidAmount();
+     if (_amount < queueDepositMin) revert InvalidAmount();

(uint256 minStakeAmount, ) = communityPool.getStakerLimits();
if (_amount < minStakeAmount) revert InvalidAmount();