arbitrage-pool Static Analysis Findings

ARB-01S: Inexistent Sanitization of Input Addresses

Type	Severity	Location
Input Sanitization		arbitrage-pool.sol:L33-L35, L130-L132

Description:

The linked function(s) accept address arguments yet do not properly sanitize them.

Impact:

The presence of zero-value addresses, especially in constructor implementations, can cause the contract to be permanently inoperable. These checks are advised as zero-value inputs are a common side-effect of off-chain software related bugs.

Example:

Copy
34factory = ITroveFactory(_factory);

Recommendation:

We advise some basic sanitization to be put in place by ensuring that each address specified is non-zero.

Alleviation:

The Bonq Protocol team has introduced sufficient checks validating that the input address cannot be zero. They have also removed the ability to update the router variable. We will further advise to mark the router variable as immutable.

ARB-02S: Improper Invocations of EIP-20 transfer / transferFrom

Type	Severity	Location
Standard Conformity		arbitrage-pool.sol:L63, L79, L84, L157

Description:

The linked statements do not properly validate the returned bool values of the EIP-20 standard transfer & transferFrom functions. As the standard dictates, callers must not assume that false is never returned.

Impact:

If the code mandates that the returned bool is true, this will cause incompatibility with tokens such as USDT / Tether as no such bool is returned to be evaluated causing the check to fail at all times. On the other hand, if the token utilized can return a false value under certain conditions but the code does not validate it, the contract itself can be compromised as having received / sent funds that it never did.

Example:

Copy
63IERC20(_collateralToken).transferFrom(msg.sender, address(this), _amount);

Recommendation:

Since not all standardized tokens are EIP-20 compliant (such as Tether / USDT), we advise a safe wrapper library to be utilized instead such as SafeERC20 by OpenZeppelin to opportunistically validate the returned bool only if it exists in each instance.

Alleviation:

The Bonq Protocol team has introduced the OpenZeppelin's SafeERC20 library to perform the transfer and transferFrom operations.