Skip to main content
Dedaub

Immunefi Protocol audit

Smart Contract Security Assessment

14.09.2021

Immunefi

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit on part of the Immunefi Protocol contracts, on branch “dedaub_audit2” at commit hash 250c64b0ca29853ee6d289791d1d7328ae150338. Three auditors worked over the codebase over two weeks. The audit examined:

  • the BugReportNotary and Escrow contracts
  • contracts in the distributor directory
  • contracts in the token directory
  • any other functionality as accessed by the above, but without exhaustive inspection. Specifically, the vesting contracts were inspected only as a diff from our previous audit, and contracts in lib, math, and utils were trusted.

SETTING AND CAVEATS

The exhaustively audited code base is of average size, at around 3KLoC. The extended code base is much larger, at nearly 13KLoC. The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than regular use of the protocol. Functional correctness (i.e., issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e., full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling, quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.

There are two high-level caveats that frame the rest of the report.

The first is that we audited code in relative isolation, without a complete high-level picture of how some elements are used by the rest of the protocol. This means that some functionality is certainly problematic if it is misused, but we assume it is just used appropriately. An example is the Merkle tree validation in BugReportNotary. If function discloseWithPosition is only used in the current use sites (in Escrow) then it is safe, because that use ensures that the position is not arbitrary, but one to which a previous call assigned a balance. If the function is used on its own as evidence of a verified disclosure, it is unsafe. In general, if the Merkle tree verification relies on the fullDisclose function (and not disclosure of individual leafs at non-constant positions) then the protocol is correct, relative to attacker-can-extend-Merkle-tree issues identified in our previous audit. The same observation about use of the functionality being correct under specific orchestration applies to the rest of the code.

The second caveat is that the code is complex. This is functionality that aims for maximum generality and does not shy away from complexity. The DSL for vesting controllers, the ability to get rewards in multiple assets (and the ensuing heavy nested loops), the ability to split NFT tokens, the use of multiple cloning and delegation patterns, and much more, result in conceptual complexity that requires extensive correctness reasoning, rather than the golden rule of making correctness obvious. The reentrancy avoidance (or lack of sensitivity to reentrancy) arguments in VestingNFT have troubled us significantly. Eschewing reentrancy guards (because the contract is reaching the deployment size limit of 24KB bytecode) is dangerous. For the reentrancy of the claim function alone, we feel we had to reason in terms of asynchronous distributed systems (e.g., Chandy-Misra-like algorithms) whereas a guard would have reduced the question to straightforward reasoning. Therefore, we want to recommend simplification of reasoning as a priority for the evolution of the code base. On the other hand, our own examination and the rather exhaustive testing (for reentrancy) give us a reasonable expectation of correctness for the current design.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues that affect the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed”, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

C1

Unsafe TokenBridge::onApprovalReceived and onTransferReceived

C1CRITICAL

Unsafe TokenBridge::onApprovalReceived and onTransferReceived
open

The ERC1363 Spender/Receiver callback functions are external functions that can be called by anyone, at a sensitive state. There is no check of msg.sender (or other trusted storage information) that will ensure that the callbacks are only called as part of the ERC1363 calls (e.g., transferAndCall).

For instance, consider the onTransferReceived function:

function onTransferReceived(
address, // operator
address from,
uint256 amount,
bytes memory data
) external override whenNotPaused returns (bytes4) {
(address receiver, uint256 gas, bytes memory transferData) =
abi.decode(data, (address, uint256, bytes));
bytes32 messageId = ETH_XDAI_AMB.requireToPassMessage(
address(BRIDGED_TOKEN),
abi.encodeWithSelector(IBridgedToken.bridge.selector, from, receiver,
amount, transferData),
gas
);
emit Bridged(from, receiver, amount, messageId);
return IERC1363Receiver.onTransferReceived.selector;
}

An attacker can invoke it with an arbitrary receiver, amount, etc. This will result in the attacker’s “receiver” getting any amount of tokens on the XDAI chain.

Similarly, for the onApprovalReceived function:

function onApprovalReceived(
address owner,
uint256 amount,
bytes memory data
) external override whenNotPaused returns (bytes4) {
(address receiver, uint256 gas, bytes memory transferData) =
abi.decode(data, (address, uint256, bytes));
TOKEN.safeTransferFrom(owner, address(this), amount);
bytes32 messageId = ETH_XDAI_AMB.requireToPassMessage(
address(BRIDGED_TOKEN),
abi.encodeWithSelector(IBridgedToken.bridge.selector, owner, receiver,
amount, transferData),
gas
);
emit Bridged(owner, receiver, amount, messageId);
return IERC1363Spender.onApprovalReceived.selector;
}

An attacker can invoke the function and take advantage of any existing approvals. (The actual attacker profit will be on the XDAI chain.) Such approvals are highly likely to exist, since the contract is not designed to only be used with approveAndCall, but also with a self-standing bridge function that expects an outstanding allowance.



HIGH SEVERITY

H1

Insufficient protection against tree-extension

H1HIGH

Insufficient protection against tree-extension
open

In the H1 issue of our first audit, we pointed out a potential attack vector in which a reporter submits a tree (origRoot), then the adversary "extends" it into a larger tree (advRoot) containing himself as the reporter (hence having two reporter leaves), submits it, and manages to get a payment for the extended tree. To protect against this attack and prevent payments on such extended trees, Escrow.deposit fixes the leafPosition of the reporter leaf and withdraw calls discloseWithPosition, ensuring that the reporter leaf has the expected position in the tree.

However, this defense is insufficient for the following two reasons:

    • leafPosition is a binary number in which 0 means "left" and 1 means "right". However, uint256 is padded with zeros, and the length of the proof is neither stored nor checked, hence uint256(0) does not uniquely identify a position, it could mean "0", "00", "000" and so on.

    • Even if the length of the proof is included in the check, verifying the position of the reporter leaf alone is not sufficient, since the tree can always be extended in a way such that the adversary's reporter leaf is at the expected position. A legitimate tree has the following structure, in which the reporter leaf is at position "10":

                	  origRoot
      / \
      I1 I2
      / \ / \
      number reporter name report

    The adversary can extend it in the following way:

                 advRoot
    / \
    I3 origRoot
    / \ / \
    dummy advReporter I1 I2
    / \ / \
    number reporter name report

    Note that the adversary's advReporter leaf is also at the expected position "10", so a combination of deposit/withdraw can lead to a payment for the extended advRoot. Note also that the other leaves (number, name, report) have incorrect positions in the extended tree, but their positions are not checked by deposit/withdraw.

    As a consequence, we propose all payments to require a call to fullDisclose, which has the complete tree and can safely check that no duplicate leaves exist.



MEDIUM SEVERITY

[No medium severity issues]


LOW SEVERITY

L1

Lack of reentrancy guards in VestingNFT

L1LOW

Lack of reentrancy guards in VestingNFT
open

As a design choice, VestingNFT lacks reentrancy guards in order to first, save gas, and second, reach the contract size limit. The functions have been carefully designed to tolerate reentrancy, and tests have been written to test against potential attacks.

We spent considerable time trying to understand whether the contract is indeed safe, and we could not identify specific threats. However, we also could not convince ourselves, with high confidence, that threats do not exist.

As a consequence, from a security point of view, we view the lack of reentrancy guards as a low-severity issue. There is a no-negligible risk that vulnerabilities either exist, or might be introduced in future modifications of the code (such modifications will likely receive much less attention than the initial version). Given that a simple robust defense exists (reentrancy guards), it is unclear to us whether the benefits of removing them are enough to justify the risk.

L2

Incorrect arguments in VestingNFT.claim can lead to loss of funds

L2LOW

Incorrect arguments in VestingNFT.claim can lead to loss of funds
open

When an NFT token handles multiple assets, the beneficiary can claim arbitrary amounts from each asset in every call of VestingNFT.claim. If the token is fully vested, and the beneficiary claims the full amount of a single asset, but only partially claims the rest, the remaining assets will be burned (a behaviour that is not expected from the claim function). Hence the contract relies on the correctness of the interface code to avoid loss of funds.

A potential solution could be to only burn the token if all assets have been transferred from the custodian. If only part of the tokens are fully transferred, the token can remain alive with unclaimed set to 0. When the token is vested, the claim function still allows all tokens to be transferred, even when unclaimed == 0. This prevents accidental loss of funds, while still allowing the beneficiary to manually burn the token if he wishes to do so.



OTHER/ ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

Unclear why VestingNFT::supportsInterface is noProxy

A1ADVISORY

Unclear why VestingNFT::supportsInterface is noProxy
info

It is not clear to us why this function is not callable on custodian tokens. We are compelled to ask if this has an interesting reason.

A2

Floating pragma

A2ADVISORY

Floating pragma
info

Use of a floating pragma: The floating pragma pragma solidity ^0.8.7; is used, allowing contracts to be compiled with any version of the Solidity compiler that is greater or equal to v0.8.7 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.

A3

Compiler known issues

A3ADVISORY

Compiler known issues
info

Solidity compiler v0.8.7, at the time of writing, has no known bugs. Some files (e.g., MerkleProof) use compiler version 0.8.0, which has two known bugs: "KeccakCaching” and “ABIDecodeTwoDimensionalArrayMemory”. We believe that neither of them affects the code: no multidimensional arrays seem to be used in files under 0.8.0, and no keccak hashing of constant memory arrays takes place.



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub