Farseen0/scar-pairs huggingface dataset:
LOW severity: Missing validation in `BunniQuoter` results in incorrect quotes. **Description:** `BunniQuoter::quoteDeposit` assumes that caller provided the correct vault fee; however, if a vault fee of zero is passed for a vault that has a non-zero vault fee, the returned quote will be incorrect as shown in the PoC below. Additionally, for the case where there is an...

Vulnerable code:
...
    if (existingShareSupply == 0) {
        // ensure that the added amounts are not too small to mess with the shares math
        if (addedAmount0 < MIN_DEPOSIT_BALANCE_INCREASE && addedAmount1 < MIN_DEPOSIT_BALANCE_INCREASE) {
            revert BunniHub__DepositAmountTooSmall();
        }
        // no existing shares, just give WAD
        shares = WAD - MIN_INITIAL_SHARES;
        // prevent first staker from stealing funds of subsequent stakers
        // see https://code4rena.com/reports/2022-01-sherlock/#h-01-first-user-can-steal-everyone-elses-tokens
        shareToken.mint(address(0), MIN_INITIAL_SHARES, address(0));
    } else {
        // given that the position may become single-sided, we need to handle the case where one of the existingAmount values is zero
@>      if (existingAmount0 == 0 && existingAmount1 == 0) revert BunniHub__ZeroSharesMinted();
        shares = FixedPointMathLib.min(
            existingAmount0 == 0 ? type(uint256).max : existingShareSupply.mulDiv(addedAmount0, existingAmount0),
            existingAmount1 == 0 ? type(uint256).max : existingShareSupply.mulDiv(addedAmount1, existingAmount1)
        );
        if (shares == 0) revert BunniHub__ZeroSharesMinted();
    }
    ...

function quoteWithdraw(address sender, IBunniHub.WithdrawParams calldata params)
        external
        view
        override
        returns (bool success, uint256 amount0, uint256 amount1)
    {
        PoolId poolId = params.poolKey.toId();
        PoolState memory state = hub.poolState(poolId);
        IBunniHook hook = IBunniHook(address(params.poolKey.hooks));

++      IAmAmm.Bid memory topBid = hook.getTopBid(poolId);
++      if (hook.getAmAmmEnabled(poolId) && topBid.manager != address(0) && !params.useQueuedWithdrawal) {
++          return (false, 0, 0);
++      }
        ...
    }

contract ERC4626FeeMock is ERC4626 {
    address internal immutable _asset;
    uint256 public fee;
    uint256 internal constant MAX_FEE = 10000;

    constructor(IERC20 asset_, uint256 _fee) {
        _asset = address(asset_);
        if(_fee > MAX_FEE) revert();
        fee = _fee;
    }

    function setFee(uint256 newFee) external {
        if(newFee > MAX_FEE) revert();
        fee = newFee;
    }

    function deposit(uint256 assets, address to) public override returns (uint256 shares) {
        return super.deposit(assets - assets * fee / MAX_FEE, to);
    }

    function asset() public view override returns (address) {
        return _asset;
    }

    function name() public pure override returns (string memory) {
        return "MockERC4626";
    }

    function symbol() public pure override returns (string memory) {
        return "MOCK-ERC4626";
    }
}
LOW severity: Before swap delta can exceed the actual specified amount for exact input swaps due to rounding. **Description:** When returning the `beforeSwapDelta` in `BunniHookLogic::beforeSwap`, the `actualInputAmount` is assigned as the maximum between the `amountSpecified` and the `inputAmount` calculated using Bunni swap math:

The intention is to accept any excess specified amount as noted by the...

Vulnerable code:
// return beforeSwapDelta
// take in max(amountSpecified, inputAmount) such that if amountSpecified is greater we just happily accept it
int256 actualInputAmount = FixedPointMathLib.max(-params.amountSpecified, inputAmount.toInt256());
inputAmount = uint256(actualInputAmount);
beforeSwapDelta = toBeforeSwapDelta({
    deltaSpecified: actualInputAmount.toInt128(),
    deltaUnspecified: -outputAmount.toInt256().toInt128()
});

// compute input and output token amounts
// NOTE: The rounding direction of all the values involved are correct:
// - cumulative amounts are rounded up
// - naiveSwapAmountIn is rounded up
// - naiveSwapAmountOut is rounded down
// - currentActiveBalance0 and currentActiveBalance1 are rounded down
// Overall this leads to inputAmount being rounded up and outputAmount being rounded down
// which is safe.
// Use subReLU so that when the computed output is somehow negative (most likely due to precision loss)
// we output 0 instead of reverting.
(inputAmount, outputAmount) = zeroForOne
    ? (
        updatedActiveBalance0 - input.currentActiveBalance0,
        subReLU(input.currentActiveBalance1, updatedActiveBalance1)
    )
    : (
        updatedActiveBalance1 - input.currentActiveBalance1,
        subReLU(input.currentActiveBalance0, updatedActiveBalance0)
    );

return (updatedSqrtPriceX96, updatedTick, inputAmount, outputAmount);
HIGH severity: Outdated references to implementation details in `ERReferrer` should be replaced. **Description:** `ERC20Referrer` makes multiple references to balances being stored as `uint232`, with the upper 24 bits of the storage slot used to store the lock flag and referrer; however, this is outdated as balances are now stored using 255 bits, with the singular highest bit used to store...

Vulnerable code:
/// @dev Balances are stored as uint232 instead of uint256 since the upper 24 bits
/// of the storage slot are used to store the lock flag & referrer.
/// Referrer 0 should be reserved for the protocol since it's the default referrer.
abstract contract ERC20Referrer is ERC20, IERC20Referrer, IERC20Lockable {
    /// -----------------------------------------------------------------------
    /// Errors
    /// -----------------------------------------------------------------------

    /// @dev Error when the balance overflows uint232.
    error BalanceOverflow(); // @audit-info - info: not uint232 but 255 bits
    ...
}
HIGH severity: Infinite Permit2 approval is not recommended. **Description:** When creating a rebalance order in `RebalanceLogic::_createRebalanceOrder`, the `BunniHook` approves tokens to be spent by the Permit2 contract. Specifically, if the order input amount exceeds the existing allowance, the Permit2 contract is approved to spend the maximum uint for...

Vulnerable code:
// approve input token to permit2
if (inputERC20Token.allowance(address(this), env.permit2) < inputAmount) {
    address(inputERC20Token).safeApproveWithRetry(env.permit2, type(uint256).max);
}
HIGH severity: Potential for blockchain explorer griefing due to successful zero value transfers from non-approved callers. **Description:** When `ERC20Referrer::transferFrom` is invoked from an address that has no approval given by the `from` address, execution reverts:

However, if both the allowance and amount to transfer are zero then execution continues uninterrupted. Instead, additional validation should be added.....

Vulnerable code:
// Compute the allowance slot and load its value.
mstore(0x20, msgSender)
mstore(0x0c, _ALLOWANCE_SLOT_SEED)
mstore(0x00, from)
let allowanceSlot := keccak256(0x0c, 0x34)
let allowance_ := sload(allowanceSlot)
// If the allowance is not the maximum uint256 value.
if add(allowance_, 1) {
    // Revert if the amount to be transferred exceeds the allowance.
    if gt(amount, allowance_) {
        mstore(0x00, 0x13be252b) // `InsufficientAllowance()`.
        revert(0x1c, 0x04)
    }
    // Subtract and store the updated allowance.
    sstore(allowanceSlot, sub(allowance_, amount))
}

function test_transferFromPoC() external {
    uint256 amount = 1e18;
    token.mint(bob, amount, address(0));

    vm.prank(bob);
    token.approve(address(this), amount);

    // zero transfer from bob to alice doesn't revert
    vm.prank(alice);
    token.transferFrom(bob, alice, 0);
}
HIGH severity: An attacker can drain the entire protocol balance of sUSDe during the yield phase due to incorrect redemption accounting logic in `pUSDeVault::_withdraw`. **Description:** After transitioning to the yield phase, the entire protocol balance of USDe is deposited into sUSDe and pUSDe can be deposited into the yUSDe vault to earn additional yield from the sUSDe. When initiating a redemption, `yUSDeVault::_withdraw` is called which in turn invokes...

Vulnerable code:
function _withdraw(address caller, address receiver, address owner, uint256 pUSDeAssets, uint256 shares) internal override {
        if (!withdrawalsEnabled) {
            revert WithdrawalsDisabled();
        }

        if (caller != owner) {
            _spendAllowance(owner, caller, shares);
        }


        _burn(owner, shares);
@>      pUSDeVault.redeem(pUSDeAssets, receiver, address(this));
        emit Withdraw(caller, receiver, owner, pUSDeAssets, shares);
    }

function _withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares) internal override {

            if (PreDepositPhase.YieldPhase == currentPhase) {
                // sUSDeAssets = sUSDeAssets + user_yield_sUSDe
@>              assets += previewYield(caller, shares);

@>              uint sUSDeAssets = sUSDe.previewWithdraw(assets); // @audit - this rounds up because sUSDe requires the amount of sUSDe burned to receive assets amount of USDe to round up, but below we are transferring this rounded value out to the receiver which actually rounds against the protocol/yUSDe depositors!

                _withdraw(
                    address(sUSDe),
                    caller,
                    receiver,
                    owner,
                    assets, // @audit - this should not include the yield, since it is decremented from depositedBase
                    sUSDeAssets,
                    shares
                );
                return;
            }
        ...
    }

function _withdraw(
            address token,
            address caller,
            address receiver,
            address owner,
            uint256 baseAssets,
            uint256 tokenAssets,
            uint256 shares
        ) internal virtual {
            if (caller != owner) {
                _spendAllowance(owner, caller, shares);
            }
@>          depositedBase -= baseAssets; // @audit - this can underflow when redeeming yUSDe because previewYield() increments assets based on sUSDe preview but this decrement should be equivalent to the base asset amount that is actually withdrawn from the vault (without yield)

            _burn(owner, shares);
            SafeERC20.safeTransfer(IERC20(token), receiver, tokenAssets);
            onAfterWithdrawalChecks();

            emit Withdraw(caller, receiver, owner, baseAssets, shares);
            emit OnMetaWithdraw(receiver, token, tokenAssets, shares);
        }
MEDIUM severity: `MetaVault::redeemRequiredBaseAssets` should be able to redeem small amounts from each vault to fill requested amount and avoid redeeming more than requested. **Description:** `MetaVault::redeemRequiredBaseAssets` is supposed to iterate through the supported vaults, redeeming assets until the required amount of base assets is obtained:

Its implementation however only retrieves from a supported vault if that one withdrawal can satisfy the desired...

Vulnerable code:
/// @notice Iterates through supported vaults and redeems assets until the required amount of base tokens is obtained

function redeemRequiredBaseAssets (uint baseTokens) internal {
    for (uint i = 0; i < assetsArr.length; i++) {
        IERC4626 vault = IERC4626(assetsArr[i].asset);
        uint totalBaseTokens = vault.previewRedeem(vault.balanceOf(address(this)));
        // @audit only withdraw if a single withdraw can satisfy desired amount
        if (totalBaseTokens >= baseTokens) {
            vault.withdraw(baseTokens, address(this), address(this));
            break;
        }
    }
}
HIGH severity: Use `SafeER:forceApprove` instead of standard `IER:approve`. **Description:** Use [`SafeERC20::forceApprove`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L101-L108) when dealing with a range of potential tokens instead of standard `IERC20::approve`:

**Strata:** Fixed in commit...

Vulnerable code:
predeposit/yUSDeDepositor.sol
58:        pUSDe.approve(address(yUSDe), amount);

predeposit/pUSDeVault.sol
178:        USDe.approve(address(sUSDe), USDeAssets);

predeposit/pUSDeDepositor.sol
86:            asset.approve(address(vault), amount);
98:        sUSDe.approve(address(pUSDe), amount);
110:        USDe.approve(address(pUSDe), amount);
122:        token.approve(swapInfo.router, amount);
HIGH severity: `MetaVault::redeem` erroneously calls `ERUpgradeable::withdraw` when attempting to redeem `USDe` from `pUSDeVault`. **Description:** Unlike `MetaVault::deposit`, `MetaVault::mint`, and `MetaVault::withdraw` which all invoke the corresponding `IERC4626` function, `MetaVault::redeem` erroneously calls `ERC4626Upgradeable::withdraw` when attempting to redeem `USDe` from `pUSDeVault`:

**Impact:** The behavior of...

Vulnerable code:
function redeem(address token, uint256 shares, address receiver, address owner) public virtual returns (uint256) {
    if (token == asset()) {
        return withdraw(shares, receiver, owner);
    }
    ...
}

function redeem(address token, uint256 shares, address receiver, address owner) public virtual returns (uint256) {
        if (token == asset()) {
--          return withdraw(shares, receiver, owner);
++          return redeem(shares, receiver, owner);
        }
        ...
    }
HIGH severity: Duplicate vaults can be pushed to `assetsArr`. **Description:** While `MetaVault::addVault` is protected by the `onlyOwner` modifier, there is no restriction on the number of times this function can be called with a given `vaultAddress` as argument:

In such a scenario, the vault will become duplicated within the `assetsArr` array. When called.....

Vulnerable code:
function addVault(address vaultAddress) external onlyOwner {
        addVaultInner(vaultAddress);
    }

    function addVaultInner (address vaultAddress) internal {
        TAsset memory vault = TAsset(vaultAddress, EAssetType.ERC4626);
        assetsMap[vaultAddress] = vault;
@>      assetsArr.push(vault);

        emit OnVaultAdded(vaultAddress);
    }

function removeVaultAndRedeemInner (address vaultAddress) internal {
        // Redeem
        uint balance = IERC20(vaultAddress).balanceOf(address(this));
@>      if (balance > 0) {
@>          IERC4626(vaultAddress).redeem(balance, address(this), address(this));
        }

        // Clean
        TAsset memory emptyAsset;
@>      assetsMap[vaultAddress] = emptyAsset;
        uint length = assetsArr.length;
        for (uint i = 0; i < length; i++) {
            if (assetsArr[i].asset == vaultAddress) {
                assetsArr[i] = assetsArr[length - 1];
@>              assetsArr.pop();
                break;
            }
        }
    }

    /// @dev Internal method to redeem all assets from supported vaults
    /// @notice Iterates through all supported vaults and redeems their assets for the base token
    function redeemMetaVaults () internal {
        while (assetsArr.length > 0) {
@>          removeVaultAndRedeemInner(assetsArr[0].asset);
        }
    }

function requireSupportedVault(address token) internal view {
@>      address vaultAddress = assetsMap[token].asset;
        if (vaultAddress == address(0)) {
            revert UnsupportedAsset(token);
        }
    }

    function removeVault(address vaultAddress) external onlyOwner {
@>      requireSupportedVault(vaultAddress);
        removeVaultAndRedeemInner(vaultAddress);

        emit OnVaultRemoved(vaultAddress);
    }
LOW severity: `MetaVault::addVault` should enforce identical underlying base asset. **Description:** When supporting additional vaults, `MetaVault::addVault` should enforce that the new vault being supported has an identical underlying base asset as itself. Otherwise:
* `redeemRequiredBaseAssets` won't work as expected since the newly supported vault doesn't have the same base...

Vulnerable code:
function test_vaultSupportedWithDifferentUnderlyingAsset() external {
    // create ERC4626 vault with different underlying ERC20 asset
    MockUSDe differentERC20 = new MockUSDe();
    MockERC4626 newSupportedVault = new MockERC4626(differentERC20);

    // verify pUSDe doesn't have same underlying asset as new vault
    assertNotEq(pUSDe.asset(), newSupportedVault.asset());

    // but still allows it to be added
    pUSDe.addVault(address(newSupportedVault));

    // this breaks `MetaVault::redeemRequiredBaseAssets` since
    // the newly supported vault doesn't have the same base asset
}

function addVaultInner (address vaultAddress) internal {
+       IERC4626 newVault = IERC4626(vaultAddress);
+       require(newVault.asset() == asset(), "Vault asset mismatch");
LOW severity: `pUSDeVault::startYieldPhase` should not remove supported vaults from being supported or should prevent new supported vaults once in the yield phase. **Description:** The intention of `pUSDeVault::startYieldPhase` is to convert assets from existing supported vaults into `USDe` in order to then stake the vault's total `USDe` into the `sUSDe` vault.

However because this ends up calling `MetaVault::removeVaultAndRedeemInner`, all the supported...

Vulnerable code:
function test_supportedVaultsRemovedWhenYieldPhaseEnabled() external {
    // supported vault prior to yield phase
    assertTrue(pUSDe.isAssetSupported(address(eUSDe)));

    // user1 deposits $1000 USDe into the main vault
    uint256 user1AmountInMainVault = 1000e18;
    USDe.mint(user1, user1AmountInMainVault);

    vm.startPrank(user1);
    USDe.approve(address(pUSDe), user1AmountInMainVault);
    uint256 user1MainVaultShares = pUSDe.deposit(user1AmountInMainVault, user1);
    vm.stopPrank();

    // admin triggers yield phase on main vault
    pUSDe.startYieldPhase();

    // supported vault was removed when initiating yield phase
    assertFalse(pUSDe.isAssetSupported(address(eUSDe)));

    // but can be added back in?
    pUSDe.addVault(address(eUSDe));
    assertTrue(pUSDe.isAssetSupported(address(eUSDe)));

    // what was the point of removing it if it can be re-added
    // and used again during the yield phase?
}
LOW severity: `pUSDeVault::maxWithdraw` doesn't account for withdrawal pausing, in violation of EIP-4626 which can break protocols integrating with `pUSDeVault`. **Description:** [EIP-4626](https://eips.ethereum.org/EIPS/eip-4626) states on `maxWithdraw`:
> MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

`pUSDeVault::maxWithdraw` doesn't account for withdrawal pausing, in...

Vulnerable code:
function test_maxWithdraw_WhenWithdrawalsPaused() external {
    // user1 deposits $1000 USDe into the main vault
    uint256 user1AmountInMainVault = 1000e18;
    USDe.mint(user1, user1AmountInMainVault);

    vm.startPrank(user1);
    USDe.approve(address(pUSDe), user1AmountInMainVault);
    uint256 user1MainVaultShares = pUSDe.deposit(user1AmountInMainVault, user1);
    vm.stopPrank();

    // admin pauses withdrawals
    pUSDe.setWithdrawalsEnabled(false);

    // reverts as maxWithdraw returns user1AmountInMainVault even though
    // attempting to withdraw would revert
    assertEq(pUSDe.maxWithdraw(user1), 0);

    // https://eips.ethereum.org/EIPS/eip-4626 maxWithdraw says:
    // MUST factor in both global and user-specific limits,
    // like if withdrawals are entirely disabled (even temporarily) it MUST return 0
}
LOW severity: `pUSDeVault::maxDeposit` doesn't account for deposit pausing, in violation of EIP-4626 which can break protocols integrating with `pUSDeVault`. **Description:** [EIP-4626](https://eips.ethereum.org/EIPS/eip-4626) states on `maxDeposit`:
> MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0.

`pUSDeVault::maxDeposit` doesn't account for deposit pausing, in...

Vulnerable code:
function test_maxDeposit_WhenDepositsPaused() external {
    // admin pauses deposists
    pUSDe.setDepositsEnabled(false);

    // reverts as maxDeposit returns uint256.max even though
    // attempting to deposit would revert
    assertEq(pUSDe.maxDeposit(user1), 0);

    // https://eips.ethereum.org/EIPS/eip-4626 maxDeposit says:
    // MUST factor in both global and user-specific limits,
    // like if deposits are entirely disabled (even temporarily) it MUST return 0.
}
LOW severity: `pUSDeVault::maxMint` doesn't account for mint pausing, in violation of EIP-4626 which can break protocols integrating with `pUSDeVault`. **Description:** [EIP-4626](https://eips.ethereum.org/EIPS/eip-4626) states on `maxMint`:
> MUST factor in both global and user-specific limits, like if mints are entirely disabled (even temporarily) it MUST return 0.

`pUSDeVault::maxMint` doesn't account for mint pausing, in violation of...

Vulnerable code:
function test_maxMint_WhenDepositsPaused() external {
    // admin pauses deposists
    pUSDe.setDepositsEnabled(false);

    // should revert here as maxMint should return 0
    // since deposits are paused and `MetaVault::mint` uses `_deposit`
    assertEq(pUSDe.maxMint(user1), type(uint256).max);

    // attempt to mint to show the error
    uint256 user1AmountInMainVault = 1000e18;
    USDe.mint(user1, user1AmountInMainVault);

    vm.startPrank(user1);
    USDe.approve(address(pUSDe), user1AmountInMainVault);
    // reverts with DepositsDisabled since `MetaVault::mint` uses `_deposit`
    uint256 user1MainVaultShares = pUSDe.mint(user1AmountInMainVault, user1);
    vm.stopPrank();

    // https://eips.ethereum.org/EIPS/eip-4626 maxMint says:
    // MUST factor in both global and user-specific limits,
    // like if mints are entirely disabled (even temporarily) it MUST return 0.
}
LOW severity: `pUSDeVault::maxRedeem` doesn't account for redemption pausing, in violation of EIP-4626 which can break protocols integrating with `pUSDeVault`. **Description:** [EIP-4626](https://eips.ethereum.org/EIPS/eip-4626) states on `maxRedeem`:
> MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0.

`pUSDeVault::maxRedeem` doesn't account for redemption pausing, in...

Vulnerable code:
function test_maxRedeem_WhenWithdrawalsPaused() external {
    // user1 deposits $1000 USDe into the main vault
    uint256 user1AmountInMainVault = 1000e18;
    USDe.mint(user1, user1AmountInMainVault);

    vm.startPrank(user1);
    USDe.approve(address(pUSDe), user1AmountInMainVault);
    uint256 user1MainVaultShares = pUSDe.deposit(user1AmountInMainVault, user1);
    vm.stopPrank();

    // admin pauses withdrawals
    pUSDe.setWithdrawalsEnabled(false);

    // doesn't revert but it should since `MetaVault::redeem` uses `_withdraw`
    // and withdraws are paused, so `maxRedeem` should return 0
    assertEq(pUSDe.maxRedeem(user1), user1AmountInMainVault);

    // reverts with WithdrawalsDisabled
    vm.prank(user1);
    pUSDe.redeem(user1MainVaultShares, user1, user1);

    // https://eips.ethereum.org/EIPS/eip-4626 maxRedeem says:
    // MUST factor in both global and user-specific limits,
    // like if redemption are entirely disabled (even temporarily) it MUST return 0
}
LOW severity: `yUSDeVault` inherits from `PreDepositVault` but doesn't call `onAfterDepositChecks` or `onAfterWithdrawalChecks`. **Description:** `pUSDeVault` and `yUSDeVault` both inherit from `PreDepositVault`.

`pUSDeVault` uses `PreDepositVault::onAfterDepositChecks` and `onAfterWithdrawalChecks` inside its overriden `_deposit` and `_withdraw` functions.

However `yUSDeVault` doesn't do this; instead it attempts to...

Vulnerable code:
if (totalSupply() < MIN_SHARES) {
    revert MinSharesViolation();
}
HIGH severity: Inability to remove and redeem from vaults with withdrawal issues could result in a bank-run. **Description:** When deposits are made to the `pUSDeVault`, `depositedBase` is incremented based on the previewed quote amount of USDe underlying the external ERC-4626 vaults; however, these instantaneous preview quotes are not necessarily accurate when compared to the maximum amount that is...

Vulnerable code:
uint baseAssets = IERC4626(token).previewRedeem(tokenAssets);