Immunefi Protocol audit

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.

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.

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.

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.

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.

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.

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.