Immunefi

Distributor::deposit computes the withdrawAmount by comparing the balance before and after the transfer:

uint256 initialBalance = _thisBalance(token);
if (token == NATIVE_ASSET) {
  payable(receiver).sendValue(amount);
} else {
  token.safeTransfer(receiver, amount);
}
uint256 finalBalance = _thisBalance(token);
require(initialBalance > finalBalance, "Distributor: did not withdraw");
uint256 withdrawAmount = initialBalance - finalBalance;

An adversary who controls the deposit of funds to the distributor can start withdrawing, and deposit funds back to the distributor from within his receive hook. This will cause the distributor to register a possibly much smaller withdrawAmount than the amount actually withdrawn.

When used in combination with vestingNFTReceiver, an attack can be executed as follows:

• First, the adversary withdraws an amount from vesting into the distributor, by calling VestingNFTReceiver::withdraw via Distributor::call
• Then the adversary starts withdrawing the same amount from the distributor (even if the amount is larger than his own share)
• From within his receive hook, the adversary releases an equal amount (minus 1 wei) from vesting to the distributor (again by calling VestingNFTReceiver::withdraw via Distributor::call)
• As a result, the distributor registered a withdrawal of just 1 wei, and the adversary can withdraw again.

Using the above procedure, an adversary with only 1% share can withdraw all funds from the distributor in a single transaction. An exploit of this vulnerability has been implemented and will be provided together with this report.

This vulnerability can be prevented by a cross-contract lock that prevents entering VestingNFTReceiver::withdraw while Distributor::withdraw is active.

A lighter (but less robust) solution is to add the following check:

require(withdrawAmount >= amount)

One should also keep in mind a symmetric but harder to exploit vulnerability: if the victim calls Distributor::withdraw, and in his receive hook triggers some untrusted code (e.g., transfers the received funds), the adversary can do a nested Distributor::withdraw, causing the distributor to register a larger withdrawn amount for the victim that the real one (hence increasing the adversary's share). A nonReentrant guard in Distributor::withdraw prevents this.

The general recommendation at the end of C2 also applies here.

Via Distributor::call, an adversary can call VestingNFTReceiver::transferOwnership and change the ownership to himself, which then allows him to call VestingNFTReceiver::withdraw directly (not via the distributor) and receive all vesting funds.

This can be solved by removing the transferOwnership method and baking the owner into the VestingNFTReceiver during initialization.

As a general recommendation, having a general-purpose Distributor contract which allows arbitrary interactions with VestingNFTReceiver via Distributor::call, makes it much harder to design a safe interface. We recommend using a distributor contract with exactly the needed functionality, possibly even merged with VestingNFTReceiver. This would easily solve C2, and would also make it easy to add a lock that solves C1.

A Merkle Tree can be easily "extended" by an adversary only knowing its root, to form a new tree having the original one as a subtree, together with any other data chosen by the adversary.

This could hypothetically be exploited as follows: when reportTree is submitted, the adversary extends it into an advReportTree, containing the original tree and an extra "reporter=adversary" leaf, and submits it (possibly front-running the victim to get an earlier timestamp). When the victim discloses the report leaf with a proof of inclusion in reportTree, the adversary can extend this proof into a proof that the same report leaf belongs to advReportTree. A proof of the "reporter=adversary" leaf can be also provided, so the adversary could claim the bounty for himself.

Such an attack is likely to be detected during the offchain triaging/evaluation procedure. However, relying on such a detection is a weak defense strategy. Moreover, the project owner has strong incentives to participate in this attack (to claim his own bounty), so his involvement could increase the probability of success.

Such an advReportTree will contain duplicate reporter leaves, and a method to invalidate reports with duplicate keys is envisioned in the README. However, apart from not being implemented, this is again a weak defense strategy, putting the burden of defense on the victim.

A good practice to avoid such attacks is to "sign" the report leaf by including the address of the reporter in the leaf (or a hash of the reporter, if we wish to disclose the report before the reporter). The reporter leaf could still be available if we wish to be able to disclose it alone (in this case a check can be made when everything is disclosed, that the two reporter values match).

Another design solution could be to give a fixed structure to the MT. Currently, there can be any number of leaves in any order, and a proof that a leaf is present anywhere in the tree is accepted (as a consequence key labels are needed to distinguish the leaves). This could be replaced by a design where the tree necessarily has 4 leaves in a specific order (leaf 0 is the reporter, leaf 1 the report, etc), and a proof is provided that the claimed leaf is present at its expected position. This would have the following advantages:

• extending a tree into a larger one is not possible
• there is no way to have duplicate keys (since there is only one "leaf 0")
• there is no need to annotate the leaves with key labels.

On the other hand, extending this design in the future is harder, so one should think of ways to future-proof it.

See also later advisory item concerning Merkle Trees.

The following computation is incorrect:

nextInput.presentTime = input.startTime - delay;
if (unstakingStart > 0 && input.presentTime >= unstakingStart + delay) {
  nextInput.presentTime += input.presentTime - unstakingStart;
}

The time input.startTime - delay corresponds to the earliest possible starting time in case UNSTAKE hasn't been triggered yet, so the condition should be

if (unstakingStart != 0) {

In the contract BugReportNotary.sol functions disclose() and discloseCommentary() are declared external. However, they are both called from within the contract itself as this.disclose() and this.discloseCommentary(), resulting in increased gas costs, the beginning of an internal transaction, and confusion by the change of msg.sender.

Since an obligation for external calls to these functions has no special reason, we suggest that they be declared public for clarity and gas efficiency, and the “this.” calling pattern be removed.

In combinator schedule SumVesting.sol it is implicitly assumed that the result of the sub-controllers for both getVested() and getWeight() is linearly dependant on the input amount

function getVested(CommonParameters calldata input) external pure override returns (uint256 result) {
    [...]
    for (uint256 i; i < subControllers.length; i++) {
      IVestingController subController = subControllers[i];
      uint256 share = subShares[i];
      // Dedaub: should be input.amount * share/totalShares
      // Dedaub: but the division happens in the end
      nextInput.amount = share * input.amount;
      totalShares += share;
      [...]
      result += subController.getVested(nextInput);
    }
    result /= totalShares;
}

Thus the whole input amount is passed to all sub-controllers only to divide the accumulated result amount to the totalShares at the very end.
While this assumption holds in the case of simple schedules, such as CliffVesting and LinearVesting, it may not hold for more complex ones that may be added in the future.

Similarly, an inaccurate input amount is passed to the sub-controllers in functions getContext(), createInitialState() and triggerEvent().

The following test is taken from test/commentary_tests.js :

await expect(Notary.connect(Operator).submitCommentary(BYTES32_STRING));
await expect(Notary.connect(Operator).submitCommentary(BYTES32_ZERO)).to.be.reverted;

It seems that the intention of the first line is to test that submitCommentary succeeded without reverting. However this line does not really check anything, the test will pass even if submitCommentary reverts.

The correct test would be:

await expect(Notary.connect(Operator).submitCommentary(BYTES32_STRING)).not.to.be.reverted;

Many similar cases exist in commentary_tests.js, contract_tests.js and test/distributor_tests.js (and possibly elsewhere).

In the following case, adding the .not.to.be.reverted revealed logic errors in the test:

it("Validating the attestation on disclosed report `AFTER` ATTESTATION_DELAY", async function () {
    await Notary.connect(Triager).attest(reportRoot, kk, commit)
    await expect(Notary.connect(Triager).disclose(reportRoot, key, salt, value, merkleProofval))

    const increaseTime = ATTESTATION_DELAY * 60 * 60 // ATTESTION in `hour` format x 60 min x 60 sec
    await ethers.provider.send("evm_increaseTime", [increaseTime]) //  1. increase block time
    await ethers.provider.send("evm_mine") // 2. then mine the block
...

Here, disclose is executed before the ATTESTATION_DELAY so it should fail, although the test makes it look like it should succeed. The reason why the test passes is that:

• The await expect(...) line performs no checks
• Moreover this line does not wait for the transaction to finish, so although disclose is launched before moving time forward, it is executed in the future block, after the time delay, and as a consequence it succeeds.

So, if .not.to.be.reverted is added to the await expect(...) line, the test will fail, unless the line is moved after the time increase.

There are some variables in contracts Distributor.sol and TokenMinter.sol that are assigned during contract construction and could never change thereafter.

In Distributor.sol:

 /// Only settable by the initializer.
 bool public override callEnabled;
 address public override nftHolder;
 uint256 public override maxBeneficiaries

In TokenMinter.sol:

  /// This initialized by the deployer. The token is completely trusted.
  IImmunefiToken public override token;

We suggest these variables be declared immutable for clarity and gas efficiency.

The receive() hook in VestingNFT is not to be used intentionally, since ETH is received via mint(). It would be better to revert to avoid accidentally receiving ETH.

A large amount of code (MerkleTree.sol / QuickSort.sol) is aimed at constructing (rather than verifying) a MT. However, this is only used by BugReportNotary.assignNullCommentary to construct a tree for a trivial empty commentary.

This can be easily avoided by having a hard-coded constant value NULL_COMMENTARY that denotes an empty commentary. The call to discloseCommentary can be omitted in this case (or discloseCommentary can simply check that the value is empty) and NULL_COMMENTARY can be immediately set as canonical.

In vesting schedule CliffVesting.sol function _decodeParams() is supposed to return a uint256 value

function _decodeParams(bytes calldata params) internal pure returns (uint256 cliffTime) {
    cliffTime = abi.decode(params, (uint256));
}

However, this schedule requires an empty parameter list

function checkParams(CommonParameters calldata input) external pure override {
    require(input.params.length == 0);
}

All three internal functions _decodeParams(), _decodeState() and decodeContext() are never called for CliffVesting, while the later two are also never called for LinearVesting schedules.

We suggest that all unused functions be removed for clarity and gas savings. Alternatively, the current body of CliffVesting::_decodeParams should be removed.

In KeeperRewards::keeperRewards the first argument is redundant

function keeperRewards(address, uint256 value) external pure override returns (uint256) {
    return value / 1000;
}

We suggest it be removed for clarity.

Also, the constant 1000 in the same code is an arbitrary “magic constant”, best given a name to document intent.

In BugReportNotary the MerkleProof::verify function is called with different syntax. Once as:

merkleProof.verify(reportRoot, leafHash)

and once as:

MerkleProof.verify(merkleProof, commentaryRoot, leafHash)

We recommend making uniform for consistency.

The README asks for possible ways to remove ReentrancyGuard from vestingNFT. We believe that these guards are critical and advise against trying to remove them (we see no safe way to do so, while keeping the dynamic way of computing the amount of transferred tokens).

In particular, a reentrancy to mint from withdraw will directly lead to a severe loss of funds. Currently this is indirectly protected by the nonReentrant flag in _deposit and _beforeTokenTransferInner (we recommend clearly documenting the importance of these flags, to prevent them from getting accidentally removed).

The architecture stores all ERC-20 tokens (assets) in a single contract (VestingNFT), and accounting for how they are shared among many different NFTs/bounties. This is a decision that puts a significant burden on asset accounting. It should be simpler/safer to have a treasury contract that indexes assets by NFT and keeps assets entirely separate. However, the current design seems to exist in order to support ERC-20 tokens that change in number, with time. This certainly necessitates a “shares” model instead of a “separate accounts” model. It may be good to document exactly the behavior of tokens that the designer of the contract expects, with specific token examples. There are certainly token models that will not be supported by the current design, and others that are.

A more radical approach could also be to use a clone of VestingNFT for each bounty (similarly to how clones of vestingNTFReceiver are used), so that funds for each bounty are kept in a separate contract. Apart from facilitating the accounting (no need for a "shares" model), this design would likely mitigate the losses from a critical bug (the adversary could drain a single bounty but not all of them).

The function QuickSort::sort admits some simplifications/dead-code elimination. Some of these are only possible under the invariant left < right (which is true in the current uses of the function), others regardless. We highlight them in the four code comments below.

 function sort(
   bytes32[] memory arr,
   uint256 left,
   uint256 right  // Dedaub: invariant: left < right
 ) internal pure {
   uint256 i = left;
   uint256 j = right;
   if (i == j) return;  // Dedaub: dead code, under invariant
   bytes32 pivot = arr[left + (right - left) / 2];
   while (i <= j) {  // Dedaub: definitely true the first time, under invariant,
                     // loop could be a do..while
     while (arr[i] < pivot) i++;
     while (pivot < arr[j]) j--;
     if (i <= j) {  // Dedaub: always the case, no need to check
       (arr[i], arr[j]) = (arr[j], arr[i]);
       i++;
       j--;
     }
   }
   if (left < j) sort(arr, left, j);
   if (i < right) sort(arr, i, right);
 }

In the ImplOwnable contract (currently unused) if the owner calls renounceOwnership, no new owner can be installed. It is unclear whether this is intentional and whether the contract will be used in the future.

For the bytecode size issues of VestingNFT, our suggestion would be to create a VestingNFT library contract (containing all functions that do not heavily involve storage slots, such as pure functions, some views that only affect 1-2 storage slots) and have calls in VestingNFT delegate to the library versions. Shorter-term solutions might exist (e.g., removing one of the super-contracts, such as DelegateGuard, in some way) but they will not save a large contract from bumping against size limits for long.

Use of a floating pragma: The floating pragma pragma solidity ^0.8.6; is used, allowing contracts to be compiled with any version of the Solidity compiler that is greater or equal to v0.8.6 and lower than v.0.9.0. Although the differences between these versions should be small, for deployment, floating pragmas should ideally be avoided and the pragma be fixed.

Solidity compiler v0.8.6, at the time of writing, has no known bugs.