trove Static Analysis Findings

TRO-01S: Inexistent Sanitization of Input Addresses

Type	Severity	Location
Input Sanitization		trove.sol:L55-L59

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
55constructor(
56  address _factory,
57  address _token,
58  address _troveOwner
59) Ownable() {
60  _transferOwnership(_troveOwner);
61  _grantRole(OWNER_ROLE, _troveOwner);
62  _grantRole(OWNER_ROLE, _factory);
63  factory = ITroveFactory(_factory);
64  token = IERC20(_token);
65  TOKEN_PRECISION = 10**(IERC20Metadata(_token).decimals());
66  liqTokenRateSnapshot = factory.liquidationPool(_token).liqTokenRate();
67  // allow the fee recipient contract to transfer as many tokens as it wants from the trove
68  factory.stableCoin().approve(address(factory.feeRecipient()), MAX_INT);
69}

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 addresses cannot be zero.

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

Type	Severity	Location
Standard Conformity		trove.sol:L210, L255, L404, L439, L483, L537

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
210factory_cache.stableCoin().transfer(_recipient, _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 usage of OpenZeppelin's SafeERC20 library for transfer and transferFrom invocations.