Brink - Lendle

In the BrinkVaultFactory contract, The salt for CREATE2 is computed with abi.encodePacked and includes a dynamic type (string _name):

bytes32 salt = keccak256(abi.encodePacked(_asset, _name, "BrinkVault"));
brinkVault = address(
    new BrinkVault{ salt: salt }(
        _asset, _strategist, _vaultManager, _name, _symbol, _depositLimit
    )
);

When concatenating dynamic types (or mixing dynamic and fixed types) with abi.encodePacked and then hashing, different input tuples can, in edge cases, produce identical byte sequences, creating an ambiguity risk. While a collision here is unlikely, best practice is to use abi.encode, which pads and encodes values with unambiguous boundaries.

Replace the CREATE2 salt computation to use abi.encode instead of abi.encodePacked. For example: salt = keccak256(abi.encode(_asset, _name, "BrinkVault")). This approach enforces unambiguous encoding boundaries and prevents collisions caused by dynamic-type concatenation.

SOLVED: The issue was fixed in the specified commit. The salt computation now uses abi.encode instead of abi.encodePacked, which prevents ambiguous hashing risks when dealing with dynamic types like strings.

A method allows anyone to retrieve the dust asset (that could be a result of rounding errors) from the BrickVault to the vault manager:

/// @notice Forwards dust to the vaultManager
function forwardDustToVaultManager() external {
    address token = asset();
    uint256 dust = _getAssetBalance(token);

    if (dust != 0) {
        IERC20(token).safeTransfer(vaultManager, dust);
        emit DustForwardedToPaymaster(dust);
    }
}

While not the source of potential theft, it allows anyone to cause griefing when the vault strategist withdraws funds from the strategies to the contract in order to reorganize strategies, or if the strategist transfers funds directly the the contract.

Apply an access control modifier (e.g., onlyVaultManager or onlyVaultStrategist) to forwardDustToVaultManager to restrict execution to authorized roles, and integrate dust forwarding within the controlled strategy withdrawal workflow.

SOLVED: The issue was fixed in the specified commit. The forwardDustToVaultManager function now has the onlyVaultManager access control modifier, preventing unauthorized users from calling this function and causing griefing attacks.

The mint function computes _assets = previewMint(_shares), calls harvest(), then recomputes _shares = previewDeposit(_assets) and mints that recomputed value. The vault therefore may not mint the exact _shares requested by the caller, violating ERC-4626 expectations.

function mint(uint256 _shares, address _recipient) public override returns (uint256 _assets) {
    _assets = previewMint(_shares);
    ...
    harvest();
    _shares = previewDeposit(_assets);
    _deposit(depositArgs_);
}

Invoke harvest() prior to calculating _assets with previewMint(_shares), and remove the subsequent assignment _shares = previewDeposit(_assets). This adjustment ensures the function mints exactly the amount of _shares requested by the caller.

SOLVED: The issue was fixed in the specified commit. The mint function now calls harvest() prior to calculating assets with previewMint(_shares) and no longer overwrites the shares parameter with previewDeposit(assets), ensuring the function mints exactly the amount of _shares requested by the caller.

Upon receiving a new deposit, the BrickVault distributes the amount received to the different active strategies according to their corresponding weight: strategy_amount = amount_received * stragegy_weight / TOTAL_WEIGHTS. The amount is not sent directly, but rather via a token approval before calling the supply function of the strategy:

for (uint256 i; i < _numberOfStrategies;) {
    amounts_[i] = _assets.mulDiv(_weights[i], TOTAL_WEIGHT, Math.Rounding.Floor);

    asset.safeIncreaseAllowance(_strategies[i], _assets);

    IBaseStrategy(_strategies[i]).supply(amounts_[i]);

    if (asset.allowance(address(this), _strategies[i]) > 0) asset.forceApprove(_strategies[i], 0);

    unchecked {
        ++i;
    }
}

The approved amount (_assets) is the entire deposit, not just the portion allocated to that strategy (amounts_[i]). A malicious or compromised strategy could exploit this by transferring more than its intended share, up to the full deposit.

While strategies are whitelisted, which reduces the likelihood of abuse, this unrestricted allowance still poses a critical trust risk: it enables a malicious strategy to drain all deposited funds instead of only its assigned share.

Replace the unrestricted approval by calling asset.safeIncreaseAllowance(_strategies[i], amounts_[i]) instead of approving the full _assets, and immediately reset the allowance to zero using asset.forceApprove(_strategies[i], 0) after the supply to ensure that each strategy can only transfer its allocated share.

SOLVED: The issue was fixed in the specified commit. The code now uses safeIncreaseAllowance(_strategy, amounts_[i]) to approve only the allocated amount per strategy and immediately resets the allowance to zero after each supply call, preventing malicious strategies from stealing more than their intended share.

The initialize sets numberOfStrategies = _strategies.length but then uses `enumerableSet.add, which ignores duplicates. strategies.values() can be shorter than numberOfStrategies, leading to indexing mismatches and reverts.

numberOfStrategies = _strategies.length;
...
for (uint256 i; i < numberOfStrategies; ++i) {
    strategy = _strategies[i];
    ...
    strategies.add(_strategies[i]);
    _addToWhitelist(_strategies[i]);
    totalWeight += _weights[i];
}
weights = _weights;

Enforce unique entries by requiring each call to enumerableSet.add(_strategies[i]) within initialize to succeed, reverting the transaction if duplicates are detected. After insertion, update numberOfStrategies to strategies.values().length to ensure that indexing accurately reflects the current entries.

SOLVED: The issue was fixed in the specified commit, preventing duplicates.

The setStrategist function of the BrinkVault contract does not validate the input, allowing the vault manager to set the strategist to the zero address:

function setStrategist(address strategist_) external onlyVaultManager {
    strategist = strategist_;

    emit StrategistSet(strategist_);
}

This can unintentionally disable strategist-only operations.

Implement a require check in setStrategist to reject the zero address (e.g., require(strategist_ != address(0), "Invalid strategist")).

ACKNOWLEDGED: The finding was acknowledged by the Lendle team. They mentioned that:

Even if we set the strategist role to zero address, we will be able to change it after.

Public entry points (deposit, mint, withdraw, redeem) call harvest() (external calls) and compute previews before entering nonReentrant internals. harvest() iterates strategies and calls their harvest() which is an external call; this creates a reentrancy window.

A malicious (whitelisted) strategy or future harvest implementation could alter totalAssets() or state between preview and _mint/_burn, causing incorrect accounting or reentrancy attacks.

For example in the deposit function, the harvest() function is public and calls IBaseStrategy.harvest() for each strategy which could reenter if malicious.

harvest();
_shares = previewDeposit(_assets);
_deposit(depositArgs_); // _deposit is nonReentrant

Apply OpenZeppelin’s ReentrancyGuard nonReentrant modifier to the deposit, mint, withdraw, and redeem functions to ensure that harvest() and the related state changes execute within a single reentrancy-protected context.

SOLVED: The issue was fixed in the specified commit. The deposit, mint, withdraw, and redeem functions now have the nonReentrant modifier applied, ensuring that harvest() and the related state changes execute within a single reentrancy-protected context.

The vault zips weights (an array) with strategies.values() (from EnumerableSet) and assumes index i matches the same strategy across the codebase. EnumerableSet.values() does not guarantee a stable or input-defined order. After operations like clearing and re-adding, or in the presence of duplicates during initialize, this implicit ordering can change or arrays can have mismatched lengths, leading to incorrect per-strategy allocations, withdrawals, or reporting.

If order changes, deposits/withdrawals apply weights to the wrong strategies, getBrinkVaultData() may return an address/weight pairing that changes over time

Example scenario (see the PoC section for details):

• Start with two strategies [s1, s2] and weights [60%, 40%].

• Rebalance withdraws from s1 and redeposits 50/50 into [s2, s1] (for the demonstration purpose, the strategies are not provided in the original order). The vault clears and re-adds the set, flipping the zipped order to [s2, s1]. Weights are recomputed to other values like [46%, 54%], and now index 0 maps to s2.

• Because weights are recomputed together with re-adding, the arrays happen to re-align. This is not a vulnerability in the current state of the codebase but could turn to one with a future upgrade overlooking this behaviour.

Code Location

• initialize sets numberOfStrategies from the raw input and then uses a set (deduping silently), risking length/ordering mismatch:

function initialize(address[] calldata _strategies, uint256[] calldata _weights) external onlyVaultManager {
    // ...
    numberOfStrategies = _strategies.length;
    // ...
    for (uint256 i; i < numberOfStrategies; ++i) {
        // ...
        strategies.add(_strategies[i]);
        _addToWhitelist(_strategies[i]);
        totalWeight += _weights[i];
    }
    // ...
    weights = _weights;
    initialized = true;
    emit Initialized(_strategies, _weights);
}

• Public getters and flows rely on parallel indexing of strategies.values() and weights:

DepositArgs memory depositArgs_ = DepositArgs(
    _recipient,
    _assets,
    _shares,
    numberOfStrategies,
    strategies.values(),
    weights
);

• Zipping in the core loop:

for (uint256 i; i < _numberOfStrategies;) {
    amounts_[i] = _assets.mulDiv(_weights[i], TOTAL_WEIGHT, Math.Rounding.Floor);
    asset.safeIncreaseAllowance(_strategies[i], _assets);
    IBaseStrategy(_strategies[i]).supply(amounts_[i]);
    if (asset.allowance(address(this), _strategies[i]) > 0) asset.forceApprove(_strategies[i], 0);
    unchecked { ++i; }
}

• Rebalance recomputes weights, then clears and re-adds strategies, which can alter the set’s iteration order:

newWeights[length - 1] = TOTAL_WEIGHT - totalAssignedWeight;
// update BV data
weights = newWeights;
// update strategy addresses set
strategies.clear();
for (uint256 i = 0; i < _finalStrategies.length; ++i) {
    strategies.add(_finalStrategies[i]);
}
numberOfStrategies = length;

The following test file shows the flip in the strategy indices:

// @ts-nocheck
import { expect } from "chai";
import { ethers } from "hardhat";

describe("Security PoCs", function () {
  it("[M-02] Reliance on EnumerableSet.values() ordering affects allocation targets", async function () {
    const [deployer, vaultManager, strategist, user] = await ethers.getSigners();

    // Deploy mock underlying and aToken
    const MockERC20 = await ethers.getContractFactory("MockERC20");
    const underlying = await MockERC20.deploy("MockUSDC", "mUSDC");
    await underlying.waitForDeployment();
    const aToken = await MockERC20.deploy("MockaToken", "maUSDC");
    await aToken.waitForDeployment();

    // Deploy mock reserve and two Aave strategies pointing to it
    const MockAaveReserve = await ethers.getContractFactory("MockAaveReserve");
    const reserve = await MockAaveReserve.deploy(
      await underlying.getAddress(),
      await aToken.getAddress(),
    );
    await reserve.waitForDeployment();

    const BrinkVault = await ethers.getContractFactory("BrinkVault");
    const brink = await BrinkVault.deploy(
      await underlying.getAddress(),
      strategist,
      vaultManager,
      "Test Brink",
      "TSTBrink",
      ethers.parseUnits("1000000", 6),
    );
    await brink.waitForDeployment();

    const AaveStrategy = await ethers.getContractFactory("AaveStrategy");
    const s1 = await AaveStrategy.deploy(
      await brink.getAddress(),
      await underlying.getAddress(),
      await reserve.getAddress(),
    );
    await s1.waitForDeployment();
    const s2 = await AaveStrategy.deploy(
      await brink.getAddress(),
      await underlying.getAddress(),
      await reserve.getAddress(),
    );
    await s2.waitForDeployment();

    // Initialize vault with [s1, s2] and weights [60%, 40%]
    await brink.connect(vaultManager).initialize([s1, s2], [6_000, 4_000]);

    // Fund user and deposit
    const depositAmt = ethers.parseUnits("1000", 6);
    await underlying.mint(user.address, depositAmt);
    await underlying.connect(user).approve(brink, depositAmt);
    await brink.connect(user).deposit(depositAmt, user.address);

    // Also capture protocol-side balances per strategy (aToken balances) BEFORE rebalance
    const aTokenContract = await ethers.getContractAt("ERC20", await aToken.getAddress());
    const s1BalBefore = await aTokenContract.balanceOf(await s1.getAddress());
    const s2BalBefore = await aTokenContract.balanceOf(await s2.getAddress());

    console.log("Strategy balances BEFORE (aToken)");
    console.log(`  s1: ${s1BalBefore.toString()}`);
    console.log(`  s2: ${s2BalBefore.toString()}`);

    // Verify initial order and capture weights
    let [addressesBefore, weightsBefore] = await brink.getBrinkVaultData();
    const s1Addr = await s1.getAddress();
    const s2Addr = await s2.getAddress();
    expect(addressesBefore[0]).to.equal(s1Addr);
    expect(addressesBefore[1]).to.equal(s2Addr);

    console.log("Zipped order BEFORE:");
    console.log(
      `  [0] ${addressesBefore[0]} -> ${(Number(weightsBefore[0]) / 100).toFixed(2)}%`
    );
    console.log(
      `  [1] ${addressesBefore[1]} -> ${(Number(weightsBefore[1]) / 100).toFixed(2)}%`
    );

    const wBeforeByAddr: Record<string, number> = {
      [addressesBefore[0]]: Number(weightsBefore[0]),
      [addressesBefore[1]]: Number(weightsBefore[1]),
    };

    console.log("Before rebalance:");
    console.log(
      `  s1 ${s1Addr} weight: ${(wBeforeByAddr[s1Addr] / 100).toFixed(2)}%`
    );
    console.log(
      `  s2 ${s2Addr} weight: ${(wBeforeByAddr[s2Addr] / 100).toFixed(2)}%`
    );

    // Compute expected post-rebalance balances (math based on implementation):
    // Withdraw 20% from s1, then redeposit 50/50 to [s2, s1]
    const withdrawBps = 2000n; // 20%
    const tenK = 10000n;
    const s1WithdrawAmt = (s1BalBefore * withdrawBps) / tenK;
    const half = s1WithdrawAmt / 2n;
    const s1ExpectedAfter = s1BalBefore - s1WithdrawAmt + half;
    const s2ExpectedAfter = s2BalBefore + half;

    console.log("Expectation:");
    console.log(
      `  Withdraw 20% of s1 (${s1WithdrawAmt.toString()}), then +50%/+50% => +${half.toString()} to each`
    );
    console.log(`  s1 expected after: ${s1ExpectedAfter.toString()}`);
    console.log(`  s2 expected after: ${s2ExpectedAfter.toString()}`);

    // Trigger a rebalance that intentionally flips the order in the set:
    // - withdraw 20% from s1
    // - redistribute 50/50 to [s2, s1] (order s2 first)
    const rebalanceArgs = {
      strategiesRebalanceFrom: [s1],
      weightsRebalanceFrom: [2_000],
      strategiesRebalanceTo: [s2, s1],
      weightsOfRedestribution: [5_000, 5_000],
    };
    await expect(brink.connect(strategist).rebalance(rebalanceArgs)).to.emit(
      brink,
      "RebalanceComplete",
    );

    // Protocol-side balances AFTER rebalance
    const s1BalAfter = await aTokenContract.balanceOf(await s1.getAddress());
    const s2BalAfter = await aTokenContract.balanceOf(await s2.getAddress());
    console.log("Strategy balances AFTER (aToken)");
    console.log(`  s1: ${s1BalAfter.toString()}`);
    console.log(`  s2: ${s2BalAfter.toString()}`);

    // Order and weights are derived from set.values() zipped with weights
    let [addressesAfter, weightsAfter] = await brink.getBrinkVaultData();
    // The order should now be [s2, s1]
    expect(addressesAfter[0]).to.equal(s2Addr);
    expect(addressesAfter[1]).to.equal(s1Addr);

    console.log("Zipped order AFTER (flipped):");
    console.log(
      `  [0] ${addressesAfter[0]} -> ${(Number(weightsAfter[0]) / 100).toFixed(2)}%`
    );
    console.log(
      `  [1] ${addressesAfter[1]} -> ${(Number(weightsAfter[1]) / 100).toFixed(2)}%`
    );

    const wAfterByAddr: Record<string, number> = {
      [addressesAfter[0]]: Number(weightsAfter[0]),
      [addressesAfter[1]]: Number(weightsAfter[1]),
    };

    console.log("After rebalance (order flipped):\n  - Same strategy addresses, but new percentages computed from balances.");
    console.log(
      `  s1 ${s1Addr} weight: ${(wAfterByAddr[s1Addr] / 100).toFixed(2)}%`
    );
    console.log(
      `  s2 ${s2Addr} weight: ${(wAfterByAddr[s2Addr] / 100).toFixed(2)}%`
    );

    // Sanity: actual balances match expectation
    expect(s1BalAfter).to.equal(s1ExpectedAfter);
    expect(s2BalAfter).to.equal(s2ExpectedAfter);

    // This demonstrates that the vault pairs `weights` with `strategies.values()` order.
    // After rebalance, the order flipped to [s2, s1], and any subsequent operation
    // that zips these two arrays will follow this new order, showing reliance on ordering.
    expect(weightsAfter.length).to.equal(2);
    expect([Number(weightsAfter[0]), Number(weightsAfter[1])].reduce((a,b)=>a+b,0)).to.equal(10000);
  });
}); 

The result shows the reordering:

❯ npx hardhat test --grep "Reliance on EnumerableSet"


  Security PoCs
Strategy balances BEFORE (aToken)
  s1: 600000000
  s2: 400000000
Zipped order BEFORE:
  [0] 0xCE9a1f44156c84F1c1E3b0685E15e23c51bb6d0A -> 60.00%
  [1] 0x7b1E1C3dBCDCD0F0c01FCcbbA56Ad1745A30B2Ee -> 40.00%
Before rebalance:
  s1 0xCE9a1f44156c84F1c1E3b0685E15e23c51bb6d0A weight: 60.00%
  s2 0x7b1E1C3dBCDCD0F0c01FCcbbA56Ad1745A30B2Ee weight: 40.00%
Expectation:
  Withdraw 20% of s1 (120000000), then +50%/+50% => +60000000 to each
  s1 expected after: 540000000
  s2 expected after: 460000000
Strategy balances AFTER (aToken)
  s1: 540000000
  s2: 460000000
Zipped order AFTER (flipped):
  [0] 0x7b1E1C3dBCDCD0F0c01FCcbbA56Ad1745A30B2Ee -> 46.00%
  [1] 0xCE9a1f44156c84F1c1E3b0685E15e23c51bb6d0A -> 54.00%
After rebalance (order flipped):
  - Same strategy addresses, but new percentages computed from balances.
  s1 0xCE9a1f44156c84F1c1E3b0685E15e23c51bb6d0A weight: 54.00%
  s2 0x7b1E1C3dBCDCD0F0c01FCcbbA56Ad1745A30B2Ee weight: 46.00%
    ✔ [M-02] Reliance on EnumerableSet.values() ordering affects allocation targets (816ms)


  1 passing (820ms)

Replace the combined use of EnumerableSet.values() and a separate weights array with a single array of (strategy, weight) structs. Update this array atomically during initialization and rebalancing to ensure stable, aligned ordering.

SOLVED: The issue was fixed in the specified commit. The code now uses a single StrategyConfig array that maintains stable strategy-weight pairings instead of relying on separate arrays that could become misaligned.