Farseen0/scar-pairs huggingface dataset:
HIGH severity: Use `SafeER` functions instead of standard `ER` transfer functions. **Description:** Use [`SafeERC20`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol) functions instead of standard `ERC20` transfer functions:

**Remora:** Fixed in commit...

Vulnerable code:
$ rg "transferFrom" && rg "transfer\("
RWAToken/DividendManager.sol
317:        $._stablecoin.transferFrom(

RWAToken/RemoraToken.sol
220:     * @dev Calls OpenZeppelin ERC20Upgradeable transferFrom function.
251:        return super.transferFrom(from, to, value);
344:            $._stablecoin.transferFrom(sender, $._wallet, _transferFee);
352:     * @dev Calls OpenZeppelin ERC20Upgradeable transferFrom function.
358:    function transferFrom(
376:            $._stablecoin.transferFrom(sender, $._wallet, _transferFee);
379:        return super.transferFrom(from, to, value);

TokenBank.sol
261:        IERC20(stablecoin).transferFrom(

PledgeManager.sol
196:        IERC20(stablecoin).transferFrom(

RemoraIntermediary.sol
172:        IERC20(data.assetReceived).transferFrom(
177:        IERC20(data.assetSold).transferFrom(
197:        IERC20(data.assetReceived).transferFrom(
238:        IERC20(data.paymentToken).transferFrom(
269:            IERC20(data.paymentToken).transferFrom(
296:        IERC20(data.paymentToken).transferFrom(
331:        IERC20(token).transferFrom(payer, recipient, amount);
RWAToken/DividendManager.sol
409:            $._stablecoin.transfer(holder, payoutAmount);
429:        stablecoin.transfer($._wallet, valueToClaim);

RWAToken/RemoraToken.sol
401:        $._stablecoin.transfer(account, burnPayout);

TokenBank.sol
185:        IERC20(tokenAddress).transfer(to, amount);
206:        if (amount != 0) IERC20(stablecoin).transfer(to, amount);
237:        IERC20(stablecoin).transfer(custodialWallet, totalValue);
266:        IERC20(tokenAddress).transfer(to, amount);

PledgeManager.sol
237:                _stablecoin.transfer(feeWallet, feeValue);
239:            _stablecoin.transfer(destinationWallet, amount);
299:        IERC20(stablecoin).transfer(signer, _fixDecimals(refundAmount + fee));
LOW severity: Fee refund can lose precision. **Description:** `PledgeManager::refundTokens` calculates the user fee refund as:

**Impact:** The fee refund will be less than it should be due to [division before multiplication](https://dacian.me/precision-loss-errors#heading-division-before-multiplication)

**Recommended Mitigation:** Perform...

Vulnerable code:
fee = (userPay.fee / userPay.tokensBought) * numTokens;

fee = userPay.fee * numTokens / userPay.tokensBought;
LOW severity: Changing stablecoin on TokenBank can mess up fees collection. **Description:** The feeAmount on each token is computed with the decimals of the current stablecoin (initially, a stablecoin of 6 decimals). If the stablecoin is changed to another one that uses decimals != than 6, if there are any pending fees before changing the stablecoin, those pending fees...

Vulnerable code:
function buyToken(
        address tokenAddress,
        uint32 amount
    ) external nonReentrant {
        ...
        uint64 feeValue = (stablecoinValue * curData.saleFee) / 1e6;

        ...
        curData.feeAmount += feeValue;

        IERC20(stablecoin).transferFrom(
            to,
            address(this),
            stablecoinValue + feeValue
        );
        ...
    }

    function claimAllFees() external nonReentrant restricted {
       ...
        for (uint i = 0; i < developments.length; ++i) {
//@audit => Pending fees before stablecoin was changed were computed with the decimals of the old stablecoin
            totalValue += tokenData[developments[i]].feeAmount;
            tokenData[developments[i]].feeAmount = 0;
        }
        IERC20(stablecoin).transfer(custodialWallet, totalValue);
    }

function claimAllFees() external nonReentrant restricted {
        ...
-       IERC20(stablecoin).transfer(custodialWallet, totalValue);
+       IERC20(stablecoin).transfer(custodialWallet, _fixDecimals(totalValue));
        ...
    }

//@audit => Add this function to normalize values to decimals of the current stablecoin
    function _fixDecimals(uint256 value) internal view returns (uint256) {
        return
            stablecoinDecimals < 6
                ? value / (10 ** (6 - stablecoinDecimals))
                : value * (10 ** (stablecoinDecimals - 6));
    }
LOW severity: `TokenBank::removeToken` reverts when token balance is zero, making it impossible to remove tokens from the `developments` array. **Description:** When removing a token from the TokenBank, the removal attempt reverts if it has zero tokens on the TokenBank balance. As time passes the `developments` array will grow having unnecessary tokens inside it that are impossible to remove.

**Impact:** The `developments` array will...

Vulnerable code:
function removeToken(address tokenAddress) external restricted {
        ...
-        withdrawTokens(tokenAddress, custodialWallet, 0);
+       if(IERC20(tokenAddress).balanceOf(address(this)) > 0) withdrawTokens(tokenAddress, custodialWallet, 0);
        ...
    }
LOW severity: `DividendManager::distributePayout` records a new payout record increasing the current payout index for zero `payoutAmount`. **Description:** `DividendManager::distributePayout` records a new payout record increasing the current payout index for zero `payoutAmount`.

**Proof of Concept:**

**Recommended Mitigation:** `DividendManager::distributePayout` should revert when `payoutAmount == 0`.

**Remora:** Fixed in commit.....

Vulnerable code:
function test_distributePayout_ZeroPayout() external {
    // setup one user
    address user = users[0];
    uint256 userRemoraTokens = 1;

    // whitelist user
    remoraTokenProxy.addToWhitelist(user);
    assertTrue(remoraTokenProxy.isWhitelisted(user));

    // mint user their tokens
    remoraTokenProxy.mint(user, userRemoraTokens);
    assertEq(remoraTokenProxy.balanceOf(user), userRemoraTokens);

    // allowlist user
    allowListProxy.allowUser(user, true, true, false);
    assertTrue(allowListProxy.allowed(user));

    // zero payout distribution - should revert here
    remoraTokenProxy.distributePayout(0);
    // didn't revert but increased current payout index
    assertEq(remoraTokenProxy.getCurrentPayoutIndex(), 1);
}
LOW severity: `PaymentSettler::claimAllPayouts` doesn't validate input `tokens` addresses are legitimate contracts before calling `adminClaimPayout` on them. **Description:** In `PaymentSettler::initiateBurning` and `distributePayment`, before calling any functions on the input `token` address this check occurs to ensure it is a legitimate address:

But in `PaymentSettler::claimAllPayouts` this check does not occur:

**Impact:** An attacker can deploy...

Vulnerable code:
if (!tokenData[token].active) revert InvalidTokenAddress();

function claimAllPayouts(address[] calldata tokens) external nonReentrant {
    address investor = msg.sender;
    uint256 totalPayout = 0;
    for (uint i = 0; i < tokens.length; ++i) {
        TokenData storage curToken = tokenData[tokens[i]];
        uint256 amount = IRemoraToken(tokens[i]).adminClaimPayout(
            investor,
            true
        );
LOW severity: Minting new PropertyTokens close to the end of the distribution period will dilute rewards for holders who were holding for the full period. **Description:** Holder's balanceHistory is updated each time a transfer of tokens occurs (mint or burn too). The [accounting saves the holders' balance during a certain index](https://github.com/remora-projects/remora-smart-contracts/blob/main/contracts/RWAToken/DividendManager.sol#L533-L540),...

Vulnerable code:
function _updateHolders(address from, address to) internal {
        ...
            } else {
                // else update status data and create new entry
                tHolderStatus.mostRecentEntry = payoutIndex;
//@audit => saves the holder balance for the current payout, regardless of how much time is left for the distribution to end
                $._balanceHistory[to][payoutIndex] = TokenBalanceChange({
                    isValid: true,
                    tokenBalance: toBalance
                });
            }
        }
LOW severity: Forwarder can be set to frozen address. **Description:** Forwarder can be set to frozen address; this is not ideal and can result in lost tokens in a worst-case scenario.

**Proof of Concept:**

**Recommended Mitigation:** `DividendManager::setPayoutForwardAddress` should revert if `forwardingAddress` is frozen. When an address is...

Vulnerable code:
function test_freezeHolder_setPayoutForwardAddress_toFrozenForwarder() external {
        address user1 = users[0];
        address forwarder = users[1];
        uint256 amountToMint = 1;

        _whitelistAndMintTokensToUser(user1, amountToMint);
        _whitelistAndMintTokensToUser(forwarder, amountToMint);

        // freeze forwarder
        remoraTokenProxy.freezeHolder(forwarder);

        // forward user1 payouts to frozen address
        remoraTokenProxy.setPayoutForwardAddress(user1, forwarder);

        uint64 payoutDistributionAmount = 100e6;
        _fundPayoutToPaymentSettler(payoutDistributionAmount);

        // user1 must have 0 payout because it is forwarding to `forwarder`
        uint256 user1PayoutBalance = remoraTokenProxy.payoutBalance(user1);
        assertEq(user1PayoutBalance, 0);

        // forwarder is frozen  so receives no forwarded payout
        uint256 forwarderPayoutBalance = remoraTokenProxy.payoutBalance(forwarder);
        assertEq(forwarderPayoutBalance, 0);

        // remove the forwarding while forwarder still frozen
        remoraTokenProxy.removePayoutForwardAddress(user1);

        // user1 can't claim any tokens - user1 has lost their payouts
        user1PayoutBalance = remoraTokenProxy.payoutBalance(user1);
        assertEq(user1PayoutBalance, 0);
    }
LOW severity: Impossible to remove a document added with zero uri length. **Description:** Impossible to remove a document added with zero uri length.

**Proof of Concept:** After fixing the bug "Don't add duplicate documentHash to DocumentManager::DocumentStorage::_docHashes when overwriting via _setDocument as this causes panic revert when calling _removeDocument",...

Vulnerable code:
function test_setDocumentOverwrite(string calldata uri) external {
        bytes32 docName = "0x01234";
        bytes32 docHash = "0x5555";
        bool needSignature = true;

        // add the document
        _setDocument(docName, uri, docHash, needSignature);

        // verify its hash has been added to `_docHashes`
        DocumentStorage storage $ = _getDocumentStorage();
        assertEq($._docHashes.length, 1);
        assertEq($._docHashes[0], docHash);

        // ovewrite it
        _setDocument(docName, uri, docHash, needSignature);
        // verify overwriting doesn't duplicate the hash in `_docHashes`
        assertEq($._docHashes.length, 1);
        assertEq($._docHashes[0], docHash);

        // now attempt to remove it
        _removeDocument(docHash);
    }
LOW severity: `PledgeManager::pricePerToken` can only support a maximum price of `$4294`. **Description:** `PledgeManager::pricePerToken` uses `uint32` and indicates in the comment it represents USD price using 6 decimals of precision:

**Impact:** Since the maximum value of `uint32` is 4294967295, with 6 decimals of precision the maximum USD `pricePerToken` is limited to...

Vulnerable code:
uint32 public pricePerToken; //in usd (6 decimals)
HIGH severity: Retrieve and enforce token decimal precision. **Description:** Retrieve and enforce token decimal precision using [`IERC20Metadata`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/IERC20Metadata.sol). For example:

1) `PledgeManager::initialize`

2) `TokenBank::initialize`

**Remora:** This.....

Vulnerable code:
constructor(
        address authority,
        address _holderWallet,
        address _propertyToken,
        address _stablecoin,
-       uint16 _stablecoinDecimals,
        uint32 _fundingGoal,
        uint32 _deadline,
        uint32 _withdrawDuration,
        uint32 _pledgeFee,
        uint32 _earlySellPenalty,
        uint32 _pricePerToken
    ) AccessManaged(authority) ReentrancyGuardTransient() {
        holderWallet = _holderWallet;
        propertyToken = _propertyToken;
        stablecoin = _stablecoin;
-       stablecoinDecimals = _stablecoinDecimals;
+       stablecoinDecimals = IERC20Metadata(_stablecoin).decimals();
        fundingGoal = _fundingGoal;
        deadline = _deadline;
        postDeadlineWithdrawPeriod = _withdrawDuration;
        pledgeFee = _pledgeFee;
        earlySellPenalty = _earlySellPenalty;
        pricePerToken = _pricePerToken;
        tokensSold = 0;
    }

stablecoin = _stablecoin; //must be 6 decimal stablecoin
+       require(IERC20Metadata(stablecoin).decimals() == 6, "Wrong decimals");
HIGH severity: `LockUpManager::LockUpStorage::_regLockUpTime` is never used. **Description:** `LockUpManager::LockUpStorage::_regLockUpTime` is never used:

Either remove it or add a comment noting it will be used in the future but is currently not used.

**Remora:** Fixed in commit...

Vulnerable code:
$ rg "_regLockUpTime"
RWAToken/LockUpManager.sol
36:        uint32 _regLockUpTime; //lock up time for foreign to domestic trades
HIGH severity: Use timestamp instead of uri length to test of existing document in `DocumentManager`. **Description:** Use timestamp instead of uri length to test of existing document in `DocumentManager`:

**Remora:** Fixed in commit [77af634](https://github.com/remora-projects/remora-smart-contracts/commit/77af634d953ce3549a33e7b34db924233dc7689a).

**Cyfrin:** Verified.

\clearpage

Vulnerable code:
-       if (bytes($._documents[docHash].docURI).length == 0)
-           revert EmptyDocument();
+       if ($._documents[docHash].timestamp == 0) revert EmptyDocument();
LOW severity: Future epoch cache manipulation via `calcAndCacheStakes` allows reward manipulation. **Description:** The `AvalancheL1Middleware::calcAndCacheStakes` function lacks epoch validation, allowing attackers to cache stake values for future epochs. This enables permanent manipulation of reward calculations by locking in current stake values that may become stale by the time those epochs.....

Vulnerable code:
function calcAndCacheStakes(uint48 epoch, uint96 assetClassId) public returns (uint256 totalStake) {
    uint48 epochStartTs = getEpochStartTs(epoch); // No validation of epoch timing
    // ... rest of function caches values for any epoch, including future ones
}

function getOperatorStake(
    address operator,
    uint48 epoch,
    uint96 assetClassId
) public view returns (uint256 stake) {
    if (totalStakeCached[epoch][assetClassId]) {
        uint256 cachedStake = operatorStakeCache[epoch][assetClassId][operator];
        return cachedStake;
    }
    ...
}

function test_operatorStakeOfTwoEpochsShouldBeEqual() public {
      uint256 operatorStake = middleware.getOperatorStake(alice, 1, assetClassId);
      console2.log("Operator stake (epoch", 1, "):", operatorStake);

      middleware.calcAndCacheStakes(5, assetClassId);
      uint256 newStake = middleware.getOperatorStake(alice, 2, assetClassId);
      console2.log("New epoch operator stake:", newStake);
      assertGe(newStake, operatorStake);

      uint256 depositAmount = 100_000_000_000_000_000_000;

      collateral.transfer(staker, depositAmount);

      vm.startPrank(staker);
      collateral.approve(address(vault), depositAmount);
      vault.deposit(staker, depositAmount);
      vm.stopPrank();

      vm.warp((5) * middleware.EPOCH_DURATION());

      middleware.calcAndCacheStakes(5, assetClassId);

      assertEq(
          middleware.getOperatorStake(alice, 4, assetClassId), middleware.getOperatorStake(alice, 5, assetClassId)
      );
  }
HIGH severity: Blacklisted implementation versions are accessible through migrations. **Description:** The `VaultFactory` contract implements a blacklisting mechanism to prevent the use of vulnerable or deprecated contract versions. However, the blacklisting check is only applied when creating new vaults through the create function, but is entirely absent from the migrate...

Vulnerable code:
// MockTokenizedVaultV3.sol
  function _migrate(uint64 oldVersion, uint64 newVersion, bytes calldata data) internal virtual onlyInitializing {
        // if (newVersion - oldVersion > 1) {
        //    revert();
        // }
        uint256 b_ = abi.decode(data, (uint256));
        b = b_;
    }

function testBlacklistDoesNotBlockMigration() public {
        // First, create a vault with version 1
        vault1 = vaultFactory.create(
            1, // version
            alice,
            abi.encode(
                IVaultTokenized.InitParams({
                    collateral: address(collateral),
                    burner: address(0xdEaD),
                    epochDuration: 7 days,
                    depositWhitelist: false,
                    isDepositLimit: false,
                    depositLimit: 0,
                    defaultAdminRoleHolder: alice,
                    depositWhitelistSetRoleHolder: alice,
                    depositorWhitelistRoleHolder: alice,
                    isDepositLimitSetRoleHolder: alice,
                    depositLimitSetRoleHolder: alice,
                    name: "Test",
                    symbol: "TEST"
                })
            ),
            address(delegatorFactory),
            address(slasherFactory)

        );

        // Verify initial version
        assertEq(IVaultTokenized(vault1).version(), 1);

        // Blacklist version 2
        vaultFactory.blacklist(2);

        // Despite version 2 being blacklisted, we can still migrate to it!
        vm.prank(alice);
        // This should revert if blacklist was properly enforced, but it won't
        vaultFactory.migrate(vault1, 2, abi.encode(20));

        // Verify the vault is now at version 2, despite it being blacklisted
        assertEq(IVaultTokenized(vault1).version(), 2);

        // set the value of b inside MockVaultTokenizedV3 as 20
        assertEq(
            MockVaultTokenizedV3(vault1).version2State(),
            20);
    }
HIGH severity: In `DelegatorFactory` new entity can be created for a blacklisted implementation. **Description:** The current implementation of `DelegatorFactory::create` fails to verify whether the specified implementation type has been explicitly blacklisted. This creates a security and consistency risk where blacklisted contract types can still be deployed, potentially bypassing governance.....

Vulnerable code:
// SPDX-License-Identifier: MIT
// SPDX-FileCopyrightText: Copyright 2024 ADDPHO

pragma solidity 0.8.25;

import {Test, console2} from "forge-std/Test.sol";
import {DelegatorFactory} from "../src/contracts/DelegatorFactory.sol";
import {IDelegatorFactory} from "../src/interfaces/IDelegatorFactory.sol";
import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
import {IEntity} from "../../src/interfaces/common/IEntity.sol";
import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
import "@openzeppelin/contracts/utils/introspection/IERC165.sol";

contract DelegatorFactoryTest is Test {
    address owner;
    address operator1;
    address operator2;

    DelegatorFactory factory;
    MockEntity mockImpl;

    function setUp() public {
        owner = address(this);
        operator1 = makeAddr("operator1");
        operator2 = makeAddr("operator2");

        factory = new DelegatorFactory(owner);

        // Deploy a mock implementation that conforms to IEntity
        mockImpl = new MockEntity(address(factory), 0);

        // Whitelist the implementation
        factory.whitelist(address(mockImpl));
    }

    function testCreateBeforeBlacklist() public {
        bytes memory initData = abi.encode("test");

        address created = factory.create(0, initData);

        assertTrue(factory.isEntity(created), "Entity should be created and registered");
    }

    function testCreateFailsAfterBlacklist() public {
        bytes memory initData = abi.encode("test");

        factory.blacklist(0);

        factory.create(0, initData); //@note no revert although blacklisted
    }
}

contract MockEntity is IEntity, ERC165 {
    address public immutable FACTORY;
    uint64 public immutable TYPE;

    string public data;

    constructor(address factory_, uint64 type_) {
        FACTORY = factory_;
        TYPE = type_;
    }

    function initialize(
        bytes calldata initData
    ) external {
        data = abi.decode(initData, (string));
    }

    function supportsInterface(
        bytes4 interfaceId
    ) public view virtual override(ERC165, IERC165) returns (bool) {
        return interfaceId == type(IEntity).interfaceId || super.supportsInterface(interfaceId);
    }
}
HIGH severity: Incorrect reward claim logic causes loss of access to intermediate epoch rewards. **Description:** In the current implementation of `Rewards::distributeRewards`, all shares are calculated for participants after a 3-epoch delay between the current epoch and the one being distributed. However, an issue arises in the **claim logic**.

When rewards are claimed for a past epoch, the.....

Vulnerable code:
if (totalRewards == 0) revert NoRewardsToClaim(msg.sender);
IERC20(rewardsToken).safeTransfer(recipient, totalRewards);
lastEpochClaimedOperator[msg.sender] = currentEpoch - 1; // <-- Incorrectly skips intermediate epochs

function test_distributeRewards_claimFee(uint256 uptime) public {
    uint48 epoch = 1;
    uptime = bound(uptime, 0, 4 hours);

    _setupStakes(epoch, uptime);
    _setupStakes(epoch + 2, uptime);

    address[] memory operators = middleware.getAllOperators();
    uint256 batchSize = 3;
    uint256 remainingOperators = operators.length;

    vm.warp((epoch + 3) * middleware.EPOCH_DURATION());
    while (remainingOperators > 0) {
        vm.prank(REWARDS_DISTRIBUTOR_ROLE);
        rewards.distributeRewards(epoch, uint48(batchSize));
        remainingOperators = remainingOperators > batchSize ? remainingOperators - batchSize : 0;
    }

    vm.warp((epoch + 4) * middleware.EPOCH_DURATION());

    for (uint256 i = 0; i < operators.length; i++) {
        uint256 operatorShare = rewards.operatorShares(epoch, operators[i]);
        if (operatorShare > 0) {
            vm.prank(operators[i]);
            rewards.claimOperatorFee(address(rewardsToken), operators[i]);
            assertGt(rewardsToken.balanceOf(operators[i]), 0, "Operator should receive rewards ");
            vm.stopPrank();
            break;
        }
    }

    vm.warp((epoch + 5) * middleware.EPOCH_DURATION());
    remainingOperators = operators.length;
    while (remainingOperators > 0) {
        vm.prank(REWARDS_DISTRIBUTOR_ROLE);
        rewards.distributeRewards(epoch + 2, uint48(batchSize));
        remainingOperators = remainingOperators > batchSize ? remainingOperators - batchSize : 0;
    }

    vm.warp((epoch + 9) * middleware.EPOCH_DURATION());
    for (uint256 i = 0; i < operators.length; i++) {
        uint256 operatorShare = rewards.operatorShares(epoch + 2, operators[i]);
        if (operatorShare > 0) {
            vm.prank(operators[i]);
            rewards.claimOperatorFee(address(rewardsToken), operators[i]);
            vm.stopPrank();
            break;
        }
    }
}