Farseen0/scar-pairs huggingface dataset:
HIGH severity: Redundant recomputation of accrued rewards in `StakingVault::_updateRewards`. **Description:** In [`StakingVault::_updateRewards`](https://github.com/0xKaizenLabs/staking-contracts-v3/blob/c78653ed5f2e5a6d5ace13c303a8765fe30679b0/src/StakingVault.sol#L291-L295), `accrued` is calculated and then the exact same expression is recomputed for `totalRewardDebt`. The second...

Vulnerable code:
uint256 accrued = (staked * _acc) / _scale;
if (accrued > userInfo.totalRewardDebt) {
    userInfo.storedPendingRewards += accrued - userInfo.totalRewardDebt;
}
// @audit Recomputed unnecessarily
userInfo.totalRewardDebt = (staked * _acc) / _scale;

uint256 accrued = (staked * _acc) / _scale;
if (accrued > userInfo.totalRewardDebt) {
    userInfo.storedPendingRewards += accrued - userInfo.totalRewardDebt;
}
userInfo.totalRewardDebt = accrued;
HIGH severity: Consider adding a minimum deposit amount in `StakingVault`. **Description:** [`StakingVault.deposit`](https://github.com/0xKaizenLabs/staking-contracts-v3/blob/c78653ed5f2e5a6d5ace13c303a8765fe30679b0/src/StakingVault.sol#L140-L141) accepts any positive `amount`, including 1 wei. Extremely small stakes don’t make economic sense and can amplify...

Vulnerable code:
error DepositAmountTooSmall(uint256 amount, uint256 minDeposit);

uint256 public minDeposit; // set by admin (e.g., during initialize), adjustable via setter + event

function setMinDeposit(uint256 newMin) external onlyRole(ADMIN_ROLE) {
    minDeposit = newMin;
    emit MinDepositUpdated(newMin);
}

function deposit(uint256 amount, uint64 lockStakeDuration) external whenNotPaused nonReentrant returns (uint256) {
    if (amount < minDeposit) revert DepositAmountTooSmall(amount, minDeposit);
    // ...existing logic...
}
HIGH severity: Inconsistent `recipient` handling for claimable pools. **Description:** [`L2RevenueDistributorV3.Pool.recipient`](https://github.com/0xKaizenLabs/staking-contracts-v3/blob/c78653ed5f2e5a6d5ace13c303a8765fe30679b0/src/L2RevenueDistributorV3.sol#L73) is documented as “vault address or claimable recipient key”:

However,...

Vulnerable code:
address recipient; // vault address or claimable recipient key
HIGH severity: `SignatureValidator::setAllowlist` is unrestricted leading to free purchases of tokens. **Description:** `SignatureValidator::setAllowlist` is unrestricted.

Since `SignatureValidator` inherited by `ReferralManager` and `TokenBank` this has downstream consequences.

For `TokenBank` in particular this means that
- attacker can call `TokenBank::setAllowlist(maliciousAllowList)` where...

Vulnerable code:
contract MaliciousAllowlist {

    address maliciousSigner;

    constructor(address _maliciousSigner) {
        maliciousSigner = _maliciousSigner;
    }

    function isSigner(address signer) public view returns (bool) {
        return (signer == maliciousSigner);
    }

}

function test_cyfrin_buyTokenOCP_for_free() public {
        uint64 TOTAL_TOKENS = 10_000;
        _addCentralToTokenBank(60e6, true, 50_000);

        centralTokenProxy.mint(address(tokenBankProxy), uint64(TOTAL_TOKENS));
        address attacker = getDomesticUser(2);

        (address attackerSigner, uint256 sk) = makeAddrAndKey("BUY_SIGNER");


        /*
         * Attacker sets a malicious Allowlist and signs the buy instead of a Remora Admin
         */
        vm.startPrank(attackerSigner);
        MaliciousAllowlist maliciousAllowlist = new MaliciousAllowlist(attackerSigner);
        tokenBankProxy.setAllowlist(address(maliciousAllowlist));
        bytes32 typeHash = keccak256("BuyToken(address investor, address token, uint256 amount)");
        bytes32 structHash = keccak256(abi.encode(typeHash, attacker, address(centralTokenProxy), uint256(TOTAL_TOKENS)));
        bytes32 digest = MessageHashUtils.toTypedDataHash(tokenBankProxy.getDomainSeparator(), structHash);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(sk, digest);
        bytes memory sig = abi.encodePacked(r, s, v);
        vm.stopPrank();

        /*
         * Now attacker buys all the tokens using the signature they created
         */
        vm.prank(attacker);
        tokenBankProxy.buyTokenOCP(attackerSigner, address(centralTokenProxy), TOTAL_TOKENS, sig);
        // attacker receives domestic child tokens
        assertEq(d_childTokenProxy.balanceOf(attacker), TOTAL_TOKENS);
    }

-    function setAllowlist(address allowlist) external {
+    function _setAllowlist(address allowlist) internal {
        if (allowlist == address(0)) revert InvalidAddress();
        SVStorage storage $ = _getSVStorageStorage();
        if ($._allowlist != allowlist) {
            $._allowlist = allowlist;
            emit AllowlistSet(allowlist);
        }
    }
HIGH severity: Pending payouts when resolving an investor are lost because there is no mechanism to claim the resolved payment. **Description:** When resolving a user who loses access to their account, the `ChildToken::resolveUser` function migrates balances, locks, and frozen tokens to a new address, ensuring the investor retains their previous balances. Even the pending payouts are calculated and assigned to the new...

Vulnerable code:
///DividendManager.sol//
    function _resolvePay(address oldAddress, address newAddress) internal {
//@audit-issue => no function allows the `newAddress` to claim the payout of the `oldAddress` saved on the `_resolvedPay` mapping associated to the `newAddress`
@>      _getHolderManagementStorage()._resolvedPay[newAddress] = SafeCast.toUint128(_claimPayout(oldAddress));
        emit PaymentResolved(oldAddress, newAddress);
    }
HIGH severity: Divide before multiply loses precision in `FiveFiftyRule::_updateEntityAllowance` and leads to caps being exceeded. **Description:** The function `_updateEntityAllowance` contains

This should be `REMOTE_PERCENT_DENOMINATOR * amount / aData.equity` otherwise precision loss occurs.

Since `_updateEntityAllowance` is used to calculate the amount by which the allowance should be reduced by when `add == false` this.....

Vulnerable code:
(REMORA_PERCENT_DENOMINATOR / aData.equity) * amount

@@ -505,16 +505,16 @@ contract FiveFiftyRule is UUPSUpgradeable, AccessManagedUpgradeable {
         // to side changes
         if (to != address(0)) {
             IndividualData storage iTo = individualData[to];
-
+
             if (iTo.isEntity) { // if entity
-                if (entityData[to].allowance <= amount) {
-                    entityData[to].allowance -= SafeCast.toUint64(amount);
+                if (entityData[to].allowance >= amount) {
+                    entityData[to].allowance -= SafeCast.toUint64(amount);

                     iTo.lastBalance += SafeCast.toUint64(amount);
                     emit FiveFiftyApproved(from, to, amount);
                     return true;
                 } else revert ();

*** INVARIANT VIOLATED ***
  exposure:   1200001000000
  cap amount: 1000000000000
HIGH severity: Updating the entity allowance when the individual belongs to a group that has multiple catalysts for different entities can result in mistakenly modifying the allowance of entities where the individual is not even part of. **Description:** The problem is that entities that have nothing to do with either the sender or the receiver can end up getting their allowances modified or causing the tx to revert.

The system allows to register:
- Individuals who belong to at most one group, or none.
- Groups with multiple...

Vulnerable code:
function canTransfer(address from, address to, uint256 amount) external returns (bool) {
        ...

            if (iFrom.isEntity) {
                entityData[from].allowance += SafeCast.toUint64(amount);
//@audit-info => If `from` is on a group and that group has multiple catalysts!
            } else if (gId != 0 && groups[gId].numCatalyst != 0) {
                //@audit-info => iterates over ALL the individuals of the group `from` belongs to!
                uint256 len = groups[gId].individuals.length;
                for (uint256 i; i<len; ++i) {
                    address ind = groups[gId].individuals[i];
//@audit-issue => ENTERS If the current individual of the group (doesn't matter if this individual is the `from`) is a catalyst on at least one entity
@>                  if (individualData[ind].numCatalyst != 0)
                        _updateEntityAllowance(true, ind, amount);
                }
            }
        ...

    function _updateEntityAllowance(bool add, address inv, uint256 amount) internal returns (bool) {
//@audit-info => total times the individual is a catalyst on != entities!
        uint8 numCatalyst = individualData[inv].numCatalyst;

//@audit-info => Entities the individual belongs to!
        uint256 len = findEntity[inv].length;

 //@audit-info => iterates over the entities the individual belongs to
        for (uint256 i; i<len; ++i) {
//@audit-info => if individual is not a catalyst on any entity, break out of the loop!
            if (numCatalyst == 0) break;

//@audit-info => Loads the entity data the individual belongs to on the current indx being iterated
            EntityData storage aData = entityData[findEntity[inv][i]];

//@audit-info => if the catalyst of the entity is not the investor, continue to the next entity!
            if (aData.catalyst != inv) continue;

 //@audit-info => If reaches here it means the investor is the catalyst of the entity!

            --numCatalyst;

            uint64 adjusted_amt = SafeCast.toUint64(
                (REMORA_PERCENT_DENOMINATOR / aData.equity) * amount
            );
//@audit-issue => Modifies allowance or could cause a revert on an entity that has nothing to do with the actual `from` individual because the catalyst of that entity belonged to the same group as the `from` individual, which caused execution to get here, regardless that `from` individual is not even part of the entity being modified
            if (add) aData.allowance += adjusted_amt;
            else if (adjusted_amt > aData.allowance) return false;
            else aData.allowance -= adjusted_amt;
        }
        return true;
    }
MEDIUM severity: Self transfer of all child tokens results in decement of `ChildToken.totalInvestors` storage variable. **Description:** Similar to Issue [*Frontrunning call to `ChildToken::resolveUser` and transferring all of the oldUser's `childToken` balance causes the `totalInvestor` counter to be decremented twice*](#frontrunning-call-to-childtokenresolveuser-and-transferring-all-of-the-oldusers-childtoken-balan...

Vulnerable code:
function _update(
        address from,
        address to,
        uint256 value
    ) internal override {
@1>     if (from != address(0) && balanceOf(from) - value == 0) --totalInvestors;
@2>     if (to != address(0) && balanceOf(to) == 0) ++totalInvestors;
...

function test_cyfrin_selfTransferDecrements() public {
        address user = getDomesticUser(1);
        uint32 DEFAULT_LOCK_TIME = 365 days;

        // Seed: old user has 3 tokens (locked by default on mint)
        centralTokenProxy.mint(address(this), uint64(3));
        centralTokenProxy.dynamicTransfer(user, 3);
        assertEq(d_childTokenProxy.balanceOf(user), 3);
        assertEq(d_childTokenProxy.totalInvestors(), 1);

        vm.warp(block.timestamp + DEFAULT_LOCK_TIME); // warp to unlock tokens
        vm.prank(user);
        d_childTokenProxy.transfer(user, 3);

        assertEq(d_childTokenProxy.totalInvestors(), 0);
        assertEq(d_childTokenProxy.balanceOf(user), 3);

        // Do it one more time and we get a revert
        vm.warp(block.timestamp + DEFAULT_LOCK_TIME);  // warp to unlock tokens
        vm.prank(user);
        vm.expectRevert(); // expect underflow
        d_childTokenProxy.transfer(user, 3);
    }

+       if (from != to) {
            if (from != address(0) && balanceOf(from) - value == 0) --totalInvestors;
            if (to != address(0) && balanceOf(to) == 0) ++totalInvestors;
+       }

        super._update(from, to, value);
MEDIUM severity: Resolving a frozen investor causes all the pending payouts for the entire time the investor was frozen to be lost. **Description:** When an investor is frozen, they do not receive payouts while the freeze is active. However, once the sanction is lifted, the investor is entitled to receive the accumulated payouts for the entire duration of the freeze.

Resolving a frozen user does not unfreeze the investor...

Vulnerable code:
//ChildToken.sol//
    function resolveUser(address oldAddress, address newAddress) external nonReentrant restricted {
        ...
        _resolvePay(oldAddress, newAddress); // moves any unclaimed payouts to new account
        ...
    }

//DividendManager.sol//
    function _resolvePay(address oldAddress, address newAddress) internal {
//@audit => Payouts as calculated on payoutBalance() from oldAddress are migrated to newAddress
@>      _getHolderManagementStorage()._resolvedPay[newAddress] = SafeCast.toUint128(_claimPayout(oldAddress));
        emit PaymentResolved(oldAddress, newAddress);
    }

    function _claimPayout(
        address holder
    ) internal returns (uint256 payoutAmount) {
        HolderManagementStorage storage $ = _getHolderManagementStorage();
        payoutAmount = payoutBalance(holder);

        ...
    }

    function payoutBalance(address holder) public returns (uint256) {
        ...

        uint256 payoutAmount;
//@audit => Payouts for frozen investors are paid out up to the index when they were frozen
 @>     uint16 payRangeStart = rHolderStatus.isFrozen
            ? rHolderStatus.frozenIndex - 1
            : currentPayoutIndex - 1;
        ...
        for (uint16 i = payRangeStart; i >= payRangeEnd; --i) {
            ...
        }

        ...
    }

//ChildToken.sol//
    function resolveUser(address oldAddress, address newAddress) external nonReentrant restricted {
        ...
        _resolvePay(oldAddress, newAddress); // moves any unclaimed payouts to new account
        ...
//@audit => Transfer all the balance of the oldAddress
        uint256 value = balanceOf(oldAddress);
        _validateBalance(false, false, oldAddress, value);
        _validateCompliance(false, true, oldAddress, newAddress, value);
@>      super._transfer(oldAddress, newAddress, value); // tokens already locked with _newAccountSameLocks
    }
MEDIUM severity: Allowance check in `FiveFiftyRule::canTransfer` is inverted. **Description:** The following snippet from `FiveFiftyRule::canTransfer` has inverted logic.

This is also present in `FiveFiftyRule::checkCanTransfer`.

**Impact:** No transfers to entities are possible except in the rare cases that `allowance == amount`
In the other two cases the function will...

Vulnerable code:
if (iTo.isEntity) { // if entity
@>              if (entityData[to].allowance <= amount) {
                    entityData[to].allowance -= SafeCast.toUint64(amount);

                    iTo.lastBalance += SafeCast.toUint64(amount);
                    emit FiveFiftyApproved(from, to, amount);
                    return true;
                } else revert ();
            }

if (iTo.isEntity) { // if entity
-               if (entityData[to].allowance <= amount) {
+               if (entityData[to].allowance >= amount) {
                    entityData[to].allowance -= SafeCast.toUint64(amount);

if (iData.isEntity)
-           return entityData[to].allowance <= amount;
+           return entityData[to].allowance >= amount;
MEDIUM severity: `FiveFiftyRule::_removeFromEntity` will revert or not work in some cases. **Description:** The marked line in the code below is using the wrong length `len`. It should be `eLen`.

When `len > eLen` this will lead to reverts if the investor is not found when `j < eLen`.
The result is spurious reverts.

**Impact:** Functions impacted by the reverts are  `deleteEntity` and.....

Vulnerable code:
function _removeFromEntity(
        address entity,
        address[] calldata investors
    ) internal {
        uint256 len = investors.length;
        for (uint256 i; i<len; ++i) {
            address[] storage ens = findEntity[investors[i]];
            uint256 eLen = ens.length;
@>          for (uint256 j; j<len; ++j) {
                if (ens[j] == entity) {
                    if (j != eLen-1 && eLen > 1)
                        ens[j] = ens[eLen-1];
                    ens.pop();
                    break;
                }
            }
        }
    }

function test_cyfrin_removeEntity_indexBug() public {
        address user0 = getDomesticUser(0);
        address user1 = getDomesticUser(1);
        address user2 = getDomesticUser(2);
        address entity = getDomesticUser(3);


        address[] memory investors = new address[](3);
        investors[0] = user0;
        investors[0] = user1;
        investors[0] = user2;

        fiveFiftyProxy.createEntity(entity, user0, 1e6, 1_000_000, investors);

        address[] memory investorsToRemove = new address[](2);
        investorsToRemove[0] = user0;
        investorsToRemove[1] = user1;

        // since investorsToRemove.length == 2 and entities.length == 1 we get a revert
        vm.expectRevert(stdError.indexOOBError);
        fiveFiftyProxy.removeFromEntity(entity, investorsToRemove);

    }

-          for (uint256 j; j<len; ++j) {
+          for (uint256 j; j<eLen; ++j) {
MEDIUM severity: Calling `PaymentSettler::setStableCoin` can lead to inability of holders to claim funds and old stablecoin being trapped in the contract. **Description:** An admin can call `PaymentSettler::setStablecoin` at any time. As the code is currently design this should only be done when the stablecoin balance of the contract is zero, which is a very rare occurrence given that many tokens will be at various stages of the their...

Vulnerable code:
function test_cyfrin_setStablecoinCanLeadToDOS() public {
        address user0 = getDomesticUser(0);
        address user1 = getDomesticUser(1);
        address central = address(centralTokenProxy);

        centralTokenProxy.mint(address(this), 10_000);
        centralTokenProxy.dynamicTransfer(user0, 5_000);
        centralTokenProxy.dynamicTransfer(user1, 5_000);

        paySettlerProxy.initiateBurning(central);
        vm.warp(1 days + 1);

        IERC20(address(stableCoin)).approve(address(paySettlerProxy), 1_000_000e6);
        paySettlerProxy.enableBurning(central, address(this), 1_000_000e6);

        // User 0 burns their tokens
        vm.prank(user0); d_childTokenProxy.burn();

        Stablecoin newStableCoin = new Stablecoin("DAI", "DAI", type(uint256).max/1e6, 6);
        paySettlerProxy.setStablecoin(address(newStableCoin));

        vm.startPrank(user1);
        vm.expectPartialRevert(bytes4(keccak256("ERC20InsufficientBalance(address,uint256,uint256)")));
        d_childTokenProxy.burn();
    }
MEDIUM severity: Removing Individuals from groups doesn't properly decrement the `groups.numCatalyst` to track the number of catalysts on that group. **Description:** When removing an individual from a group, the `group.numCatalyst` counter only checks if the individual's `numCatalyst` is != 0; if so, it decrements `group.numCatalyst` by 1, regardless of how many entities the individual is a catalyst for.
The problem is that each individual can.....

Vulnerable code:
//FiveFiftyRule.sol//
    function createEntity(
        ...
    ) external restricted {
        ...
        uint16 gid = individualData[catalyst].groupId;
//@audit-info => The same catalyst increments the numCatalyst counter on the group each time it's added as a catalyst on a != entity!
@>      if (gid != 0) ++groups[gid].numCatalyst;

        ...
    }

    function removeIndividual(uint16 id, address individual) external restricted {
        ...
        GroupData storage gData = groups[id];
//@audit-issue => Decrements the group numCatalysts only by one, regardless of how many times the individual is a catalyst on != entities
@>      if (iData.numCatalyst != 0) --gData.numCatalyst;

        ...
    }

function test_numCatalystInGroupsPoC() public {
        // create group with one member
        address u = getDomesticUser(0);
        address u2 = getDomesticUser(1);
        address[] memory inds = new address[](2);
        inds[0] = u;
        inds[1] = u2;

        uint16 gid = 10;

        address entityA = makeAddr("entityA");
        address entityB = makeAddr("entityB");

        fiveFiftyProxy.createGroup(gid, inds);

        //@audit => Create two entities where individual `u` is the catalyst
        fiveFiftyProxy.createEntity(entityA, u, 100, 100, inds);
        fiveFiftyProxy.createEntity(entityB, u, 100, 100, inds);

        assertEq(fiveFiftyProxy.getGroupNumCatalyst(gid), 2);

        //@audit => Remove the only user of the group who is a numCatalyst on entities
        fiveFiftyProxy.removeIndividual(gid, u);
        //@audit-issue => The numCatalyst is left as 1 even though any of the remaining individuals in the group is a catalyst on an entity
        assertEq(fiveFiftyProxy.getGroupNumCatalyst(gid), 1);
    }
LOW severity: Frontrunning call to `ChildToken::resolveUser` and transferring all of the oldUser's `childToken` balance causes the `totalInvestor` counter to be decremented twice. **Description:** When resolving a user to migrate his tokens, lock progress, and payouts from an address to a new address, the balance of `ChildToken` of the `oldAddress` is transferred by directly calling the `super:_transfer`, which bypasses the overrides of the `super::_transfer`. Calling...

Vulnerable code:
//ChildToken.sol//

    function resolveUser(address oldAddress, address newAddress) external nonReentrant restricted {
        ...
 @>     super._transfer(oldAddress, newAddress, value); // tokens already locked with _newAccountSameLocks
    }

    function _update(
        address from,
        address to,
        uint256 value
    ) internal override {
 @>     if (from != address(0) && balanceOf(from) - value == 0) --totalInvestors;
        if (to != address(0) && balanceOf(to) == 0) ++totalInvestors;

        ...
    }

function test_resolveUser_FrontRanHijacks_totalInvestorCounter() public {
        address oldUser = getDomesticUser(1);
        address newUser = getDomesticUser(2);
        address extraInvestor = getDomesticUser(3);

        centralTokenProxy.mint(address(this), uint64(1));
        centralTokenProxy.dynamicTransfer(extraInvestor, 1);

        // Seed: old user has 3 tokens (locked by default on mint)
        centralTokenProxy.mint(address(this), uint64(3));
        centralTokenProxy.dynamicTransfer(oldUser, 3);
        assertEq(d_childTokenProxy.balanceOf(oldUser), 3);

        // Distribute payout via PaymentSettler -> Central -> Child
        IERC20(address(stableCoin)).approve(address(paySettlerProxy), type(uint256).max);
        paySettlerProxy.distributePayment(address(centralTokenProxy), address(this), 300); // 300 USD(6) total

        assertEq(d_childTokenProxy.totalInvestors(), 2);

        uint32 DEFAULT_LOCK_TIME = 365 days;
        vm.warp(block.timestamp + DEFAULT_LOCK_TIME);

        //@audit-info => `oldUser` frontruns `resolveUser()` and transfers all of his balance!
        vm.prank(oldUser);
        d_childTokenProxy.transfer(newUser, 3);

        //@audit-info => Doesn't revert even though `oldUser` has 0 balance
        // Resolve: move state to newUser (already allowlisted + signed in base)
        d_childTokenProxy.resolveUser(oldUser, newUser);

        //@audit-issue => Only 1 investor when in reality are 2 (newUser and extraInvestor)
        assertEq(d_childTokenProxy.totalInvestors(), 1);

        //@audit-info => newUser transfers all his tokens to extraInvestor - totalInvestors shrinks
        vm.warp(block.timestamp + DEFAULT_LOCK_TIME);
        vm.prank(newUser);
        d_childTokenProxy.transfer(extraInvestor, 3);
        assertEq(d_childTokenProxy.totalInvestors(), 0);

        //@audit-issue => underflow because totalInvestors is 0 and extraInvestor is transferring all of his balance
        vm.warp(block.timestamp + DEFAULT_LOCK_TIME);
        vm.prank(extraInvestor);
        vm.expectRevert();
        d_childTokenProxy.transfer(newUser, 4);
    }