Farseen0/scar-pairs huggingface dataset:
HIGH severity: Rewards calculation error will result in 0 rewards for users. **Description**

The formula to calculate rewards in `getUserStakeReward` is the following:

It is the same in `updateRewardsPerWeight`. The problem with this code is that `(unaccountedTime * rewardsPerWeight_.rate) / rewardsPerWeight_.totalWeight` will round down to zero almost always. Since both.....

Vulnerable code:
rewardsPerWeight_.accumulated = (rewardsPerWeight_.accumulated +
    (unaccountedTime * rewardsPerWeight_.rate) / rewardsPerWeight_.totalWeight).toUint96();

- (unaccountedTime * rewardsPerWeight_.rate) / rewardsPerWeight_.totalWeight
+ (unaccountedTime * rewardsPerWeight_.rate).divWadDown(rewardsPerWeight_.totalWeight)

- return getUserStakeWeight(userStake) * (rewardsPerWeight_.accumulated - userStake.checkpoint);
+ return getUserStakeWeight(userStake).mulWadDown(rewardsPerWeight_.accumulated - userStake.checkpoint);
HIGH severity: Protocol fees from NFT mints can't be claimed in `BatonLaunchpad`. **Severity**

**Impact:**
High, as it results in a loss of value for the protocol

**Likelihood:**
High, as it certain to happen

**Description**

In `Nft::mint` the `msg.value` expected is the price of an NFT multiplied by the amount of NFTs to mint plus a protocol fee. This protocol fee is sent...

Vulnerable code:
if (protocolFee != 0) {
    address(batonLaunchpad).safeTransferETH(protocolFee);
}
HIGH severity: Operators can cast an extra vote to get voting majority. **Severity**

**Impact:**
High, as it breaks the `votingRatio` invariant

**Likelihood:**
High, as operators can cast an extra vote at any time

**Description**

In `PoolGovernance::proposeEpoch`, operators can cast an extra vote when this is needed to get to `votingRatio`. Exploiting this, a...

Vulnerable code:
bytes32 prevVote = votes[epochNumber][msg.sender];
uint256 count = ++voteCounter[epochNumber][vote];
uint256 operatorsLen = operators.length;

votes[epochNumber][msg.sender] = vote;

if (prevVote != bytes32(0)) --voteCounter[epochNumber][prevVote];

it("Operators can cast an extra vote to get voting majority", async () => {
  await governance.addOperators([
    operator1.address,
    operator2.address,
    operator3.address,
  ]);
  await time.increase(week);
  await governance
    .connect(operator1)
    .proposeEpoch([withdrawals.root, exits.root, state, fee]);

  expect(await governance.epochNumber()).to.equal(0);
  // operator1 casts a second vote to get 66% vote ratio
  await governance
    .connect(operator1)
    .proposeEpoch([withdrawals.root, exits.root, state, fee]);

  // validate that the epoch increased (vote passed)
  expect(await governance.epochNumber()).to.equal(1);
});

bytes32 prevVote = votes[epochNumber][msg.sender];
- uint256 count = ++voteCounter[epochNumber][vote];
uint256 operatorsLen = operators.length;

votes[epochNumber][msg.sender] = vote;

if (prevVote != bytes32(0)) --voteCounter[epochNumber][prevVote];
+ uint256 count = ++voteCounter[epochNumber][vote];
MEDIUM severity: Operator might be unable to withdraw rewards due to gas limit. **Severity**

**Impact:**
High, as operator's yield will be frozen

**Likelihood:**
Low, as it requires operator to be a special multi-sig wallet or contract

**Description**

The `PoolGovernance:withdrawRewards` method allows operators to withdraw their yield, which happens with this external...

Vulnerable code:
(bool sent, ) = msg.sender.call{value: rewards, gas: 2300}("");
LOW severity: Timestamp comparison has a logical error. In `WERC721::transferFromWithAuthorization` we see the following code:

There is a logical error there, as even though the variable is named `validAfter`, the value of the variable would still work as a valid timestamp for transfer. Example is if the code says "validAfter = 12:00:00 PM" and then...

Vulnerable code:
if (block.timestamp < validAfter) revert InvalidTransferAuthorization();
if (block.timestamp > validBefore)
    revert InvalidTransferAuthorization();
HIGH severity: Recoveries can be blocked by front-running. **Severity**

**Impact:**
High, as recovery functionality won't be usable

**Likelihood:**
High, as there are no preconditions to executing such an attack on all recoveries

**Description**

Each call from `AmbireAccount` to `DKIMRecoverySigValidator::validateSig` can be front-runned by calling...

Vulnerable code:
require(!recoveries[identifier], 'recovery already done');
HIGH severity: Recoveries of type `SigMode.OnlySecond` can be endlessly replayed. **Severity**

**Impact:**
High, as it can result in a loss of wallet access

**Likelihood:**
Medium, as it requires specific conditions but they can happen even after years of using the wallet

**Description**

The `recoveries` mapping in `DKIMRecoverySigValidator` is serving as a way to check if...

Vulnerable code:
bytes32 identifier = keccak256(abi.encode(accountAddr, data, sigMeta));
HIGH severity: Collateral double-spend post liquidation is possible. **Severity**

**Impact:**
High, as a borrower can double-spend his collateral

**Likelihood:**
High, as liquidations never subtract the collateral amount

**Description**

In `AssetManager::assetTransferOnLoanAction` in the `if (action == AssetActionType.Seize)` statement, the transferred amount...

Vulnerable code:
function test_LiquidateExploit() public {
    // because of the `setUp` method, at this point Bob has taken a loan from Alice through Lumin
    uint256 bobDeposit = wrappedAssetManager.depositOf(assetId[1], bob).depositAmount;

    // make loan expire
    vm.warp(block.timestamp + 300 days + 1);
    // random user liquidates Bob's loan, so collateral (asset[1]) should be transferred to the lender
    vm.prank(0x1111111111111111111111111111111111111111);
    wrappedLoanManagerDelegator.liquidate(1);

    // Bob can withdraw his whole deposit even though part of it was used as collateral and was liquidated
    uint256 bobCollateralWalletBalance = mockERC20Token[1].balanceOf(bob);
    vm.prank(bob);
    wrappedAssetManager.assetDepositWithdraw(assetId[1], IAssetManager.AssetActionType.Withdraw, bobDeposit);

    assertEq(mockERC20Token[1].balanceOf(bob) - bobCollateralWalletBalance, bobDeposit);
}

else if (action == AssetActionType.Seize) {
        userDepositFrom.lockedAmount -= amount;
+       userDepositFrom.depositAmount -= amount;
        userDepositTo.depositAmount += amount;
    }
HIGH severity: Disabled lender's loan configuration can be used by a borrower. **Severity**

**Impact:**
Medium, as a borrower can use a disabled loan configuration but it will still work as a normal loan, so no lender value loss

**Likelihood:**
High, as the disabling functionality can never work with the current code

**Description**

In the `LoanConfig` struct we have the.....

Vulnerable code:
function test_DisabledLoanConfigCanStillBeUsed() public {
    // Alice disables her loan config
    vm.prank(alice);
    wrappedLoanManagerDelegator.updateLoanConfigEnabledStatus(loan.configId, false);
    vm.startPrank(bob);

    // there are no current loans
    assertEq(0, wrappedLoanManagerDelegator.getLoanCounter());

    vm.expectEmit();
    emit LoanCreated(1, 1);
    wrappedLoanManagerDelegator.createLoan(loan, collateralAssets);

    // a loan was created
    assertEq(1, wrappedLoanManagerDelegator.getLoanCounter());

    // Alice's free deposit was lowered
    IAssetManager.DepositData memory aliceDepositAfterLending = wrappedAssetManager.depositOf(assetId[0], alice);
    assertEq(aliceDepositAfterLending.depositAmount, userDepositAlice[0] - loan.principalAmount);
    assertEq(aliceDepositAfterLending.lockedAmount, 0);
}
MEDIUM severity: Asset loan config can never be removed from its EnumerableSet. **Severity**

**Impact:**
Medium, as a user will be limited in his platform usage but won't lose value

**Likelihood:**
Medium, as it happens when a user has created the max total loan configs for an asset

**Description**

The `LoanConfigManager` contract uses the `userConfigIds` mapping to keep...

Vulnerable code:
function deleteLoanConfig(uint256 configId) external {
    LoanConfig storage config = loanConfigs[configId];
    if (config.lender != msg.sender) {
        revert NotAuthorized();
    }

    if (config.totalPendingPrincipalAmount > 0) {
        revert LoanConfigInUse();
    }

    delete (loanConfigs[configId]);
    userConfigIds[msg.sender][config.config.principalAssetId].remove(configId);

    emit LoanConfigDeleted(configId);
}

if (userConfigIds[msg.sender][config.principalAssetId].length() >= limits.maxLoanConfigsPerAsset) {
    revert MaxConfigsReached();
}

function test_AssetLoanConfigIsNotRemovedFromEnumerableSet() public {
    uint256 amount = 100_000;

    vm.startPrank(alice);
    mockERC20Token[0].approve(address(assetManagerProxy), amount);
    wrappedAssetManager.assetDepositWithdraw(
        loanConfigUser.principalAssetId, IAssetManager.AssetActionType.Deposit, amount
    );

    // Create 10 (the maximum total allowed for a single asset) loan configs for the same asset
    for (uint256 i = 0; i < 10; i++) {
        wrappedLoanManagerDelegator.createLoanConfig(loanConfigUser, loanConfigAssetUsage);
    }

    // Remove one of the loan configs
    wrappedLoanManagerDelegator.deleteLoanConfig(1);

    // Try to create another loan config for the same asset, but
    // it reverts since the count (length of the EnumerableSet) wasn't decremented
    vm.expectRevert(abi.encodeWithSelector(ILoanConfigManager.MaxConfigsReached.selector));
    wrappedLoanManagerDelegator.createLoanConfig(loanConfigUser, loanConfigAssetUsage);
}
MEDIUM severity: Price feed oracle data validation is missing. **Severity**

**Impact:**
High, as it can possibly result in unfair liquidations

**Likelihood:**
Low, as it only happens in specific rare conditions

**Description**

There are multiple problems when validating the price feed data in `PriceFeedProxyChainlink`:

- the code doesn't revert when...

Vulnerable code:
- if (intPrice < 0) {
+ if (intPrice <= 0) {
    revert ImplausiblePrice();
}
MEDIUM severity: Protocol does not support fee-on-transfer and rebasing tokens. **Severity**

**Impact:**
High, as this can leave tokens stuck in the protocol

**Likelihood:**
Low, as a small portion of the commonly used tokens have such mechanisms

**Description**

The `_depositWithdraw` method in `AssetManager` has the following implementation:

Also before/after it is...

Vulnerable code:
function _depositWithdraw(address assetAddress, bool deposit, address sender, uint256 amount) private {
    if (deposit) {
        IERC20(assetAddress).safeTransferFrom(sender, address(this), amount);
    } else {
        IERC20(assetAddress).safeTransfer(sender, amount);
    }
}

assetDeposit.depositAmount -= amount;
userDeposit.depositAmount -= amount;

assetDeposit.depositAmount += amount;
userDeposit.depositAmount += amount;
LOW severity: Using `address(0)` as a valid value. In `AssetManager::_assetAction` we can see the following line of code:

This is used to track the total deposits by any user for an asset (also special handling is implemented in `AssetManager::balancesOf`). The problem is that `address(0)` is used as a valid value here, but `address(0)` is also...

Vulnerable code:
DepositData storage assetDeposit = deposits[assetId][address(0)];
HIGH severity: Last NFT from the supply can't be minted. **Severity**

**Impact:**
Medium, as only one NFT won't be available for minting, but this is value loss to the protocol

**Likelihood:**
High, as it's impossible to mint the last NFT

**Description**

Currently both the `mint` and `mintPhysical` methods have the following check:

This is...

Vulnerable code:
if (nextId + amount >= MAX_SUPPLY) revert ExceedsMaxSupply();

function testNotAllNFTsCanBeMinted() public {
        museum.setPrice(PRICE);
        uint256 allButOneNFTSupply = 3089;

        // mint all but one from the NFT `MAX_SUPPLY` (3090)
        museum.mint{value: allButOneNFTSupply * PRICE}(address(this), allButOneNFTSupply);
        require(allButOneNFTSupply == museum.balanceOf(address(this)), "Mint did not work");

        // try to mint the last NFT from the supply, but it doesn't work
        vm.expectRevert(MuseumOfMahomes.ExceedsMaxSupply.selector);
        museum.mint{value: PRICE}(address(this), 1);
    }

- if (nextId + amount >= MAX_SUPPLY) revert ExceedsMaxSupply();
+ if (nextId + amount > MAX_SUPPLY) revert ExceedsMaxSupply();
HIGH severity: Calling `Curve::withdraw` will likely result in users losing ETH. **Severity**

**Impact:**
High, as users can lose ETH value

**Likelihood:**
High, as all >75 Curve pools that have ETH are problematic

**Description**

The `Curve::withdraw` method removes liquidity from a pool to burn LP tokens and receive the underlying assets, after which a user can sweep...

Vulnerable code:
it('User can not claim his ETH liquidity', async () => {
      const { contract, sign } = await loadFixture(deploy);

      const POOL = '0xdc24316b9ae028f1497c275eb9192a3ea0f67022';
      const POOL_TOKEN = '0x06325440d014e39736583c165c2963ba99faf14e';

      const proxyFee = 5n;
      const ethAmount = 1n * 10n ** 18n;

      const amounts = [ethAmount, 0]; // eth - steth

      const poolToken = await ethers.getContractAt(
        'IERC20',
        POOL_TOKEN,
      );

      const poolTokenBalanceBefore = await poolToken.balanceOf(
        account.address,
      );

      const addTx = await contract.populateTransaction.deposit(
        amounts,
        0,
        POOL,
        proxyFee,
      );
      const sweepTx = await contract.populateTransaction.sweepToken(
        POOL_TOKEN,
        account.address,
      );

      await contract.multicall([addTx.data, sweepTx.data], 0, {
        value: ethAmount + proxyFee,
      });

      const poolTokenBalanceAfter = await poolToken.balanceOf(
        account.address,
      );

      expect(poolTokenBalanceAfter).to.gt(poolTokenBalanceBefore);

      await poolToken.approve(PERMIT2_ADDRESS, constants.MaxUint256);

      const { permit, signature } = await sign(
        {
          amount: poolTokenBalanceAfter,
          token: POOL_TOKEN,
        },
        contract.address,
      );

      const approveTx =
        await contract.populateTransaction.approveToken(POOL_TOKEN, [
          POOL,
        ]);
      const permitTx =
        await contract.populateTransaction.permitTransferFrom(
          permit,
          signature,
        );

      const removeLiquidityTx =
        await contract.populateTransaction.withdraw(
          poolTokenBalanceAfter,
          [0, 0],
          POOL,
        );

      // remove liquidity then try with unwrapping, as if the contract wrapped the received ETH in WETH
      const unwrapTx = await contract.populateTransaction.unwrapWETH9(
        account.address,
      );

      let ethBalanceBefore = await account.getBalance();
      await contract.multicall(
        [
          approveTx.data,
          permitTx.data,
          removeLiquidityTx.data,
          unwrapTx.data,
        ],
        0,
      );

      let ethBalanceAfter = await account.getBalance();
      expect(ethBalanceBefore).to.gt(ethBalanceAfter); // balance before is actually more than current because of paying gas - it should have been ~1 ETH more, because of `ethAmount`
    });
  });
HIGH severity: The fee mechanism is not enforced. **Severity**

**Impact:**
High, as the protocol can lose potential yield in the form of fees

**Likelihood:**
High, as users can craft such transactions in a permissionless way

**Description**

The codebase is using a fee mechanism where the users pay a fee for using some functionality. An...

Vulnerable code:
function depositETHV2(address _recipient, uint256 _proxyFeeInWei) external payable nonETHReuse {
        address _cEther = address(cEther);

        ICEther(_cEther).mint{value: msg.value - _proxyFeeInWei}();
    ....
    ....
MEDIUM severity: Calls to methods with `nonETHReuse` modifier can be force reverted. **Severity**

**Impact:**
Medium, as the user will get its transaction reverted, but it can be replayed through a `Multicall` call

**Likelihood:**
Medium, as it can only happen when there is a direct call to such methods, which isn't the usual way to use the app

**Description**

In the contracts.....

Vulnerable code:
function _nonReuseBefore() private {
        // On the first call to nonETHReuse, _status will be NOT_ENTERED
        if (_status == ENTERED) {
            revert EtherReuseGuardCall();
        }

        // Any calls to nonETHReuse after this point will fail
        _status = ENTERED;
    }
LOW severity: No `withdrawTo` functionality in `AmbirePaymaster`. The ERC4337 implementation on Ethereum has a `StakeMaster` contract with a `withdrawTo` functionality, allowing a paymaster to withdraw his deposit as seen [here](https://github.com/eth-infinitism/account-abstraction/blob/674b1f51164e641a18d5c141895bab9c96a68e9d/contracts/core/StakeManager.sol#L137-...

Vulnerable code:
* @notice  This method can be used to withdraw stuck tokens or airdrops
HIGH severity: Protocol assets can be stolen through sandwich attacks. **Severity**

**Impact:**
High, as protocol assets will be lost

**Likelihood:**
High, as sandwich attacks are very common and easy to execute

**Description**

The `Capsule` contract is currently vulnerable to sandwich attack in many places. Every place which does a swap or provides/removes...

Vulnerable code:
uint256 expectedAmount = curvePoolWETHCRV.get_dy(2, 1, crvBal); // Estimate WETH received for CRV:WETH exchange
// Exchange CRV for WETH within the slippage limit
curvePoolWETHCRV.exchange(2, 1, crvBal, expectedAmount * LPslippageNum / LPslippageDen);
MEDIUM severity: User supplying a fake vault can steal protocol assets as a grief attack. **Severity**

**Impact:**
High, as a malicious actor can drain protocol assets

**Likelihood:**
Low, as it is a common attack vector and it requires no preconditions

**Description**

The `_vault` argument in the `repay` method (and in all others) has no input validation, meaning a user can supply.....

Vulnerable code:
PETH.approve(address(_vault), tokensWithdrawn); // Approve JPEGD to take PETH
_vault.repay(_id, tokensWithdrawn); // Repay desired amount of loan based on desired repayment size
ePosition.lpSize -= LPcostToRepay; // No need to adjust profit index here, previous check requires they either withdraw or forfeit profits beforehande the users amount borrowed
ePosition.amountBorrowed -= tokensWithdrawn; // Decrease the users amount borrowed
emit DecreaseBorrowAmount(owner, _nft, _id, tokensWithdrawn);
LOW severity: Penalty calculation in `previewRedeem` rounds in the wrong direction. The `previewRedeem` method in `AutoPxEth` calculates the penalty with the following code:

The issue is that the math rounds down the penalty, but following the ERC4626 standard you should follow best security for the vault and when calculating the assets a user will receive when burning shares,...

Vulnerable code:
uint256 penalty = (_totalSupply == 0 || _totalSupply - shares == 0)
    ? 0
    : assets.mulDivDown(withdrawalPenalty, FEE_DENOMINATOR);
HIGH severity: Protocol will not be compatible with commonly used ERtokens. **Severity**

**Impact:**
Medium, as it most probably won't result in value loss but it limits the protocol usability

**Likelihood:**
Medium, as it will not work with some commonly used tokens, but will work with others

**Description**

Here is a code snippet from `PumpV1::fulfill`:

The code is.....

Vulnerable code:
if (order.isBuy) {
    ERC20(order.baseToken).transferFrom(order.maker, msg.sender, baseTokenAmount - fee);
    ERC20(order.baseToken).transferFrom(order.maker, owner(), fee);
    Token(order.assetToken).transferFrom(msg.sender, order.recipient, amount);
} else {
    ERC20(order.baseToken).transferFrom(msg.sender, order.recipient, baseTokenAmount - fee);
    ERC20(order.baseToken).transferFrom(msg.sender, owner(), fee);
    Token(order.assetToken).transferFrom(order.maker, msg.sender, amount);
}
MEDIUM severity: DoS attack on initialization is possible. **Severity**

**Impact:**
High, as the initialization can be blocked

**Likelihood:**
Low, as it a front-running type of an attack with no benefit for the attacker

**Description**

The `initializeDistributor` method in `TokenDistributor` has the following check:

This gives the expectation that...

Vulnerable code:
require(token.balanceOf(address(this)) == _totalTokensToDistribute, "totalTokensToDistribute must match token balance of contract");

- require(
-    token.balanceOf(address(this)) == _totalTokensToDistribute,
-    "totalTokensToDistribute must match token balance of contract"
- );
+ token.safeTransferFrom(msg.sender, address(this), _totalTokensToDistribute);
LOW severity: Wrapped token name is the unwrapped token name. The `WrappedElon` contract serves as a wrapper for `$ELON` tokens. Here is how its constructor looks like:

The problem is that instead of naming the token with the same name, prepended with the "Wrapped" word, it is using the same name as the unwrapped version. This can lead to confusions,...

Vulnerable code:
constructor() ERC20("Dogelon", "ELON") {}

-constructor() ERC20("Dogelon", "ELON") {}
+constructor() ERC20("WrappedDogelon", "WELON") {}
LOW severity: Protocol is using a vulnerable library version. In `.gitmodules` file in the repository we can see this:

This version contains multiple vulnerabilities as you can see [here](https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories). While the problems are not present in the current codebase, it is strongly advised to upgrade...

Vulnerable code:
url = https://github.com/openzeppelin/openzeppelin-contracts
branch = release-v4.8