Farseen0/scar-pairs huggingface dataset:
MEDIUM severity: `PledgeManager::refundTokens` doesn't decrement `tokensSold` when pledge hasn't concluded, preventing pledge from reaching its funding goal. **Description:** `PledgeManager::refundTokens` doesn't decrement `tokensSold` when pledge hasn't concluded.

**Impact:** The pledge will be prevented from reaching its funding goal since the refunded tokens can't be purchased by other users.

**Recommended Mitigation:**...

Vulnerable code:
if (
            !pledgeRoundConcluded &&
            SafeCast.toUint32(block.timestamp) < deadline
        ) {
            refundAmount -= (refundAmount * earlySellPenalty) / 1e6;
            _propertyToken.adminTransferFrom(
                signer,
                holderWallet,
                numTokens,
                false,
                false
            );
            emit TokensUnPledged(signer, numTokens);
        } else {
            fee = (userPay.fee / userPay.tokensBought) * numTokens;
            _propertyToken.burnFrom(signer, numTokens);
            emit TokensRefunded(signer, numTokens);
-           tokensSold -= numTokens;
        }

+      tokensSold -= numTokens;
MEDIUM severity: `TokenBank::withdrawFunds` resets `memory` not `storage` fee and sale amounts allowing multiple withdraws for the same token. **Description:** `TokenBank::withdrawFunds` resets `memory` not `storage` fee and sale amounts allowing multiple withdraws for the same token:

**Impact:** The admin can make multiple fee and sale amount withdraws for the same token address. This will work as long as there are sufficient fee and...

Vulnerable code:
function withdrawFunds(
        address tokenAddress,
        bool fee
    ) public nonReentrant restricted {
        TokenData memory curData = tokenData[tokenAddress];
        address to;
        uint64 amount;
        if (fee) {
            to = custodialWallet;
            amount = curData.feeAmount;
            curData.feeAmount = 0; // @audit resets memory not storage
        } else {
            to = curData.withdrawTo;
            amount = curData.saleAmount;
            curData.saleAmount = 0; // @audit resets memory not storage
        }
        if (amount != 0) IERC20(stablecoin).transfer(to, amount);

        if (fee) emit FeesClaimed(tokenAddress, amount);
        else emit FundsClaimed(tokenAddress, amount);
    }

function withdrawFunds(
        address tokenAddress,
        bool fee
    ) public nonReentrant restricted {
        TokenData memory curData = tokenData[tokenAddress];
        address to;
        uint64 amount;
        if (fee) {
            to = custodialWallet;
            amount = curData.feeAmount;
-           curData.feeAmount = 0;
+           tokenData[tokenAddress].feeAmount = 0;
        } else {
            to = curData.withdrawTo;
            amount = curData.saleAmount;
-           curData.saleAmount = 0;
+           tokenData[tokenAddress].saleAmount = 0;
        }
        if (amount != 0) IERC20(stablecoin).transfer(to, amount);

        if (fee) emit FeesClaimed(tokenAddress, amount);
        else emit FundsClaimed(tokenAddress, amount);
    }
MEDIUM severity: Fee should be calculated after first purchase discount is applied in `TokenBank::buy` to prevent over-charging users. **Description:** `TokenBank::buy` calculates the total purchase amount as being composed of:
* `stablecoinValue` : value of the tokens
* `feeValue` : fee calculated off `stablecoinValue`

Afterwards if this is the user's first purchase, they receive a discount on the `stablecoinValue`:

However...

Vulnerable code:
uint64 stablecoinValue = amount * curData.pricePerToken;
        uint64 feeValue = (stablecoinValue * curData.saleFee) / 1e6;

IReferralManager refManager = IReferralManager(referralManager);
        bool firstPurchase = refManager.isFirstPurchase(to);
        if (firstPurchase) stablecoinValue -= refManager.referDiscount();

address to = msg.sender;
        uint64 stablecoinValue = amount * curData.pricePerToken;
-       uint64 feeValue = (stablecoinValue * curData.saleFee) / 1e6;

        IReferralManager refManager = IReferralManager(referralManager);
        bool firstPurchase = refManager.isFirstPurchase(to);
        if (firstPurchase) stablecoinValue -= refManager.referDiscount();

+       uint64 feeValue = (stablecoinValue * curData.saleFee) / 1e6;
        curData.saleAmount += stablecoinValue;
        curData.feeAmount += feeValue;
LOW severity: Buyers can pledge for tokens without having signed all documents that are required to be signed. **Description:** When buyers pledge() during the pledgeRound, it is verified that they [have signed all the documents](https://github.com/remora-projects/remora-smart-contracts/blob/main/contracts/PledgeManager.sol#L316) that are required to be signed.
If at least one document is not signed,...

Vulnerable code:
function _verifyDocumentSignature(
        PledgeData calldata data,
        address signer
    ) internal {
        (bool res, ) = IRemoraRWAToken(propertyToken).hasSignedDocs(signer);
        if (!res)
            IRemoraRWAToken(propertyToken).verifySignature(
                signer,
                data.docHash,
                data.signature
            );
    }

function hasSignedDocs(address signer) public view returns (bool, bytes32) {
        ...

        for (uint256 i = 0; i < numDocs; ++i) {
            bytes32 docHash = $._docHashes[i];
//@audit => If one document that needs signature is not signed, returns false
            if (
                $._documents[docHash].needSignature &&
                $._signatureRecords[signer][docHash] == 0
            ) return (false, docHash);
        }

//@audit => returns true only if all documents that requires signature are signed
        return (true, 0x0);
    }

function verifySignature(
        address signer,
        bytes32 docHash,
        bytes memory signature
    ) external returns (bool result) {
        ...
        if (signer.code.length == 0) {
            //signer is EOA
            (address returnedSigner, , ) = ECDSA.tryRecover(digest, signature);
            result = returnedSigner == signer;
        } else {
            //signer is SCA
            (bool success, bytes memory ret) = signer.staticcall(
                ...
            );
            result = (success && ret.length == 32 && bytes4(ret) == MAGICVALUE);
        }

        if (!result) revert InvalidSignature();
        if ($._signatureRecords[signer][docHash] == 0) {
           ...
        }
//@audit => if the verification of the provided signature succeeds, execution continues
    }
HIGH severity: Hardcoding deadline for `permit()` will mess up the `structHash` leading to an invalid signature. **Description:** Functions allowing users to grant ERC20 approvals via Permit incorrectly hardcode the deadline passed to permit().

The problem is that the recovered signature will be invalid because the deadline is a parameter of the structHash, and, if the signed deadline differs even by 1...

Vulnerable code:
function pledge(PledgeData calldata data) external nonReentrant {
        ...

        //5 minute deadline
        if (data.usePermit) {
            IERC20Permit(stablecoin).permit(
                signer,
                address(this),
                finalStablecoinAmount,
//@audit-issue => hardcoded deadline
                block.timestamp + 300,
                data.permitV,
                data.permitR,
                data.permitS
            );
        }
        ...
}

function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {
        if (block.timestamp > deadline) {
            revert ERC2612ExpiredSignature(deadline);
        }

//@audit-issue => A hardcoded deadline will mess up the structHash
        bytes32 structHash = keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline));

        bytes32 hash = _hashTypedDataV4(structHash);

//@audit-issue => A different hash than the actual hash signed by the signer will recover a != signer (even address(0)
        address signer = ECDSA.recover(hash, v, r, s);
        if (signer != owner) {
            revert ERC2612InvalidSigner(signer, owner);
        }

        _approve(owner, spender, value);
    }

function pledge(PledgeData calldata data) external nonReentrant {
        ...

        //5 minute deadline
        if (data.usePermit) {
            IERC20Permit(stablecoin).permit(
                signer,
                address(this),
                finalStablecoinAmount,
-              block.timestamp + 300,
+              data.deadline,
                data.permitV,
                data.permitR,
                data.permitS
            );
        }
        ...
}
MEDIUM severity: `DocumentManager::hasSignedDocs` incorrectly returns `true` when there are no documents to sign. **Description:** `DocumentManager::hasSignedDocs` incorrectly returns `true` when there are no documents to sign:

**Impact:** Upstream contracts incorrectly assume users have signed docs and allow user actions which should be prohibited.

**Proof Of Concept:**

**Recommended Mitigation:** When no.....

Vulnerable code:
function hasSignedDocs(address signer) public view returns (bool, bytes32) {
    DocumentStorage storage $ = _getDocumentStorage();
    uint256 numDocs = $._docHashes.length;

    // @audit when numDocs = 0, the `for` loop is bypassed
    // skipping to the `return (true, 0x0);` statement
    for (uint256 i = 0; i < numDocs; ++i) {
        bytes32 docHash = $._docHashes[i];
        if (
            $._documents[docHash].needSignature &&
            $._signatureRecords[signer][docHash] == 0
        ) return (false, docHash);
    }

    return (true, 0x0);
}

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.22;

import {DocumentManager} from "../../../contracts/RWAToken/DocumentManager.sol";

import {UnitTestBase} from "../UnitTestBase.sol";

contract DocumentManagerTest is UnitTestBase, DocumentManager {
    function setUp() public override {
        UnitTestBase.setUp();
        initialize();
    }

    function initialize() public initializer {
        __RemoraDocuments_init("NAME", "VERSION");
    }

    // this is incorrect and should be changed once bug is fixed
    function test_hasSignedDocs_TrueWhenNoDocs() external {
        // verify no docs
        assertEq(_getDocumentStorage()._docHashes.length, 0);

        // hasSignedDocs returns true even though no docs to sign
        (bool hasSigned, ) = hasSignedDocs(address(0x1337));
        assertTrue(hasSigned);
    }
}
MEDIUM severity: Don't add duplicate `documentHash` to `DocumentManager::DocumentStorage::_docHashes` when overwriting via `_setDocument` as this causes panic revert when calling `_removeDocument`. **Description:** `DocumentManager::_setDocument` intentionally allows overwriting but when overwriting it adds an additional duplicate `documentHash` to `_docHashes`:

**Impact:** Once the hash has been duplicated in `_docHashes`, it is impossible to remove the document by calling...

Vulnerable code:
function _setDocument(
    bytes32 documentName,
    string calldata uri,
    bytes32 documentHash,
    bool needSignature
) internal {
    DocumentStorage storage $ = _getDocumentStorage();
    $._documents[documentHash] = DocData({
        needSignature: needSignature,
        docURI: uri,
        docName: documentName,
        timestamp: SafeCast.toUint32(block.timestamp)
    });

    // @audit duplicate if overwriting
    $._docHashes.push(documentHash);
    emit DocumentUpdated(documentName, uri, documentHash);
}

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.22;

import {DocumentManager} from "../../../contracts/RWAToken/DocumentManager.sol";

import {UnitTestBase} from "../UnitTestBase.sol";

contract DocumentManagerTest is UnitTestBase, DocumentManager {
    function setUp() public override {
        UnitTestBase.setUp();
        initialize();
    }

    function initialize() public initializer {
        __RemoraDocuments_init("NAME", "VERSION");
    }

    // this is incorrect and should be changed once bug is fixed
    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);
        // this duplicates the hash in `_docHashes`
        assertEq($._docHashes.length, 2);
        assertEq($._docHashes[0], docHash);
        assertEq($._docHashes[1], docHash);

        // now attempt to remove it, reverts with
        // panic: array out-of-bounds access
        _removeDocument(docHash);
    }
}

uint256 dHLen = $._docHashes.length;
        for (uint i = 0; i < dHLen; ++i) {
            if ($._docHashes[i] == documentHash) {
                $._docHashes[i] = $._docHashes[dHLen - 1];
                $._docHashes.pop();
+               break;
            }
        }
MEDIUM severity: Check return value when calling `Allowlist::exchangeAllowed` and `RemoraToken::_exchangeAllowed` to prevent unauthorized transfers. **Description:** `Allowlist::exchangeAllowed` will revert if the users are not allowed but when the users are both allowed, it will return a boolean determined by whether the `domestic` field of both users match:

But `RemoraToken::adminTransferFrom` doesn't check the boolean return when calling...

Vulnerable code:
function exchangeAllowed(
    address from,
    address to
) external view returns (bool) {
    HolderInfo memory fromUser = _allowed[from];
    HolderInfo memory toUser = _allowed[to];
    if (from != address(0) && !fromUser.allowed)
        revert UserNotRegistered(from);
    if (to != address(0) && !toUser.allowed) revert UserNotRegistered(to);
    return fromUser.domestic == toUser.domestic; //logic to be edited later on
}

function adminTransferFrom(
    address from,
    address to,
    uint256 value,
    bool checkTC,
    bool enforceLock
) external restricted returns (bool) {
    // @audit boolean return not checked
    IAllowlist(allowlist).exchangeAllowed(from, to);
MEDIUM severity: `DividendManager::distributePayout` will always revert after 255 payouts, preventing any future payout distributions. **Description:** `DividendManager::HolderManagementStorage::_currentPayoutIndex` is declared as `uint8`:

`_currentPayoutIndex` is incremented every time `DividendManager::distributePayout` is called:

**Impact:** The maximum value of `uint8` is 255 so `DividendManager::distributePayout` can only...

Vulnerable code:
/// @dev The current index that is yet to be paid out.
uint8 _currentPayoutIndex;

$._payouts[$._currentPayoutIndex++] = PayoutInfo({
    amount: payoutAmount,
    totalSupply: SafeCast.toUint128(totalSupply())
});

mapping(address => mapping(uint8 => TokenBalanceChange)) _balanceHistory;
MEDIUM severity: Payout distributions to Remora token holders are diluted by initial token owner mint. **Description:** `RemoraToken::initialize` takes a parameter `_initialSupply` and mints this to `tokenOwner`:

`DividendManager::distributePayout` records the payout amount together with the current total supply:

`DividendManager::payoutBalance` calculates user payout amount dividing by the...

Vulnerable code:
_mint(tokenOwner, _initialSupply * 10 ** decimals());

$._payouts[$._currentPayoutIndex++] = PayoutInfo({
    amount: payoutAmount,
    totalSupply: SafeCast.toUint128(totalSupply())
});

payoutAmount +=
    (curEntry.tokenBalance * pInfo.amount) /
    pInfo.totalSupply;
MEDIUM severity: Overriding fees can't be switched back once set. **Description:** A fee is charged when buying a PropertyToken via the TokenBank. The system allows for charging a personalized fee on a per-token basis or a baseFee for all tokens.
If the variable `overrideFees` is set as true, the TokenBank will charge the baseFee that is charged to all tokens...

Vulnerable code:
function setBaseFee(
        bool updateFee,
        bool overrideFee,
        uint32 newFee
    ) external restricted {
        ...
//@audit => enters only when it is true.
//@audit => So, once set to true, it can't be changed back to false
        if (overrideFee) {
            overrideFees = overrideFee;
            emit FeeOverride(overrideFee);
        }
    }

function setBaseFee(
        bool updateFee,
        bool overrideFee,
        uint32 newFee
    ) external restricted {
        ...
-       if (overrideFee) {
            overrideFees = overrideFee;
            emit FeeOverride(overrideFee);
-       }
    }
MEDIUM severity: Pledge can't successfully complete unless `RemoraToken` is paused. **Description:** When the funding goal has been reached, `PledgeManager::checkPledgeStatus` calls `RemoraToken::unpause`:

But if `RemoraToken` is not paused, this [reverts](https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/utils/PausableUpgradeable.sol#L128).....

Vulnerable code:
function checkPledgeStatus() public returns(bool pledgeNowConcluded) {
    if (pledgeRoundConcluded) return true;

    uint32 curTime = SafeCast.toUint32(block.timestamp);
    if (tokensSold >= fundingGoal) {
        pledgeRoundConcluded = true;
        IRemoraRWAToken(propertyToken).unpause();
        emit PledgeHasConcluded(curTime);
        return true;

function test_pledge(uint256 userIndex, uint32 numTokensToBuy) external {
    address user = _getRandomUser(userIndex);
    numTokensToBuy = uint32(bound(numTokensToBuy, 1, DEFAULT_FUNDING_GOAL));

    // fund buyer with stablecoin
    (uint256 finalStablecoinAmount, uint256 fee)
        = pledgeManager.getCost(numTokensToBuy);
    stableCoin.transfer(user, finalStablecoinAmount);

    // fund this with remora tokens
    remoraTokenProxy.mint(address(this), numTokensToBuy);
    assertEq(remoraTokenProxy.balanceOf(address(this)), numTokensToBuy);

    // allow PledgeManager to spend our remora tokens
    remoraTokenProxy.approve(address(pledgeManager), numTokensToBuy);

    PledgeManagerState memory pre = _getState(address(this), user);

    remoraTokenProxy.pause();

    vm.startPrank(user);
    stableCoin.approve(address(pledgeManager), finalStablecoinAmount);
    pledgeManager.pledge(numTokensToBuy, bytes32(0x0), bytes(""));
    vm.stopPrank();

    PledgeManagerState memory post = _getState(address(this), user);

    // verify remora token balances
    assertEq(post.holderRemoraBal, pre.holderRemoraBal - numTokensToBuy);
    assertEq(post.buyerRemoraBal, pre.buyerRemoraBal + numTokensToBuy);

    // verify stablecoin balances
    assertEq(post.pledgeMgrStableBal, pre.pledgeMgrStableBal + finalStablecoinAmount);
    assertEq(post.buyerStableBal, pre.buyerStableBal - finalStablecoinAmount);

    // verify PledgeManager storage
    assertEq(post.pledgeMgrTokensSold, pre.pledgeMgrTokensSold + numTokensToBuy);
    assertEq(post.pledgeMgrTotalFee, pre.pledgeMgrTotalFee + fee);
    assertEq(post.pledgeMgrBuyerFee, pre.pledgeMgrBuyerFee + fee);
    assertEq(post.pledgeMgrTokensBought, pre.pledgeMgrTokensBought + numTokensToBuy);
}
MEDIUM severity: `RemoraToken` transfers are bricked when `from` is not whitelisted, has sufficient tokens to transfer but no tokens locked. **Description:** `RemoraToken::adminTransferFrom`, `transfer` and `transferFrom` always check if `from` is on the whitelist and if not, call `_unlockTokens`:

But a scenario can occur where:
1) `from` has nothing to unlock
2) `from` has tokens to fulfill the transfer

In this case transfer would...

Vulnerable code:
bool fromWL = whitelist[from];
    bool toWL = whitelist[to];

    if (!fromWL) _unlockTokens(from, value, false);
    if (!toWL) _lockTokens(to, value);

function test_transferBricked_fromNotWhitelisted_ButHasTokensToTransfer() external {
    address from = users[0];
    address to = users[1];
    uint256 amountToTransfer = 1;

    // fund `from` with remora tokens
    remoraTokenProxy.mint(from, amountToTransfer);
    assertEq(remoraTokenProxy.balanceOf(from), amountToTransfer);

    // remove `from` from whitelist
    remoraTokenProxy.removeFromWhitelist(from);

    // set lock time
    remoraTokenProxy.setLockUpTime(3600);

    vm.expectRevert(); // reverts with InsufficientTokensUnlockable
    vm.prank(from);
    remoraTokenProxy.transfer(to, amountToTransfer);
}

function test_transferBricked_whitelistedHolderIsRemovedFromWhitelist() external {
        address from = users[0];
        address to = users[1];
        uint256 amountToTransfer = 10;

        _whitelistAndMintTokensToUser(from, amountToTransfer);

        // set lock time
        remoraTokenProxy.setLockUpTime(3600);

        // remove `from` from whitelist
        remoraTokenProxy.removeFromWhitelist(from);

        // fund `from` with remora tokens once it is not whitelisted
        remoraTokenProxy.mint(from, amountToTransfer);

        // 10 when was whitelisted and 10 when from was not whitelisted
        assertEq(remoraTokenProxy.balanceOf(from), amountToTransfer * 2);

        // forward beyond the lockup time to demonstrate the removed whitelisted holder can't do transfers
        vm.warp(3600 + 1);

        // reverts because from has only 10 tokens locked
        vm.expectRevert(); // reverts with InsufficientTokensUnlockable
        vm.prank(from);
        remoraTokenProxy.transfer(to, 11);

        // verify from can only transfer the 10 tokens that he received after he was removed from whitelist
        vm.prank(from);
        remoraTokenProxy.transfer(to, 10);
    }
MEDIUM severity: Tokens that were locked when `lockUpTime > 0` will be impossible to unlock if `lockUpTime` is set to zero. **Description:** `LockUpManager::_unlockTokens` returns if `lockUpTime == 0`:

**Impact:** Tokens that were locked when `lockUpTime > 0` will be impossible to unlock if `lockUpTime` is subsequently set to zero. Initially this won't cause any problems and users will be able to transfer tokens as...

Vulnerable code:
function _unlockTokens(
    address holder,
    uint256 amount,
    bool disregardTime
) internal {
    LockUpStorage storage $ = _getLockUpStorage();
    uint32 lockUpTime = $._lockUpTime;
    // @audit returns if `lockUpTime == 0`
    if (lockUpTime == 0 || amount == 0) return;
MEDIUM severity: Forwarders who aren't also holders are unable to claim forwarded payouts. **Description:** Forwarders who aren't also holders are unable to claim forwarded payouts due to this check in `DividendManager::payoutBalance`:

**Impact:** Forwarders who aren't also holders are unable to claim forwarded payouts.

**Recommended Mitigation:** Remove the...

Vulnerable code:
function payoutBalance(address holder) public returns (uint256) {
        HolderManagementStorage storage $ = _getHolderManagementStorage();
        HolderStatus memory rHolderStatus = $._holderStatus[holder];
        uint16 currentPayoutIndex = $._currentPayoutIndex;

        if (
             // @audit must be a holder to claim payouts, prevents forwarders who aren't
             // also holders from claiming their forwarded payouts
            (!rHolderStatus.isHolder) || //non-holder calling the function
            (rHolderStatus.isFrozen && rHolderStatus.frozenIndex == 0) || //user has been frozen from the start, thus no payout
            rHolderStatus.lastPayoutIndexCalculated == currentPayoutIndex // user has already been paid out up to current payout index
        ) return 0;