RadarStake Static Analysis Findings

RSE-01S: Incorrect Conditional Evaluations

Type	Severity	Location
Language Specific		RadarStake.sol:L55, L90, L92, L102, L105, L133, L151, L159

Description:

The referenced conditional evaluations of unsigned integers (i.e. uint256 a) are performing comparisons that either represent tautologies (a >= 0) or impossible states (a <= 0).

Impact:

Presently, the conditionals do not sufficiently sanitize the arguments they are meant to permitting unintentional execution of certain functions such as RadarStake::addToStake with a zero-value amount.

Example:

Copy
54function addToStake(uint256 amount, address addr) external onlyStakingLogicContract {
55    require(amount >= 0, "RadarStake: Amount has to be 0 or higher");

Recommendation:

We advise the conditionals to be corrected, converting greater-than-or-equal (>=) comparisons with zero to strictly greater-than (>) and less-than-or-equal (<=) comparisons with zero to equality ones.

Alleviation (9193d77dde992ca090fe8895a88804e096173564):

All referenced conditionals except one within RadarStake::triggerUnstake have been correctly updated. While the remaining conditional should evaluate the totalStaked entry of the user to be non-zero to avoid triggering when the user has not staked yet, this is already taken care of by the RadarStakingLogic::triggerUnstake function. As such, we consider this exhibit adequately alleviated as the remaining instance is inconsequential.

RSE-02S: Inexistent Sanitization of Input Address

Type	Severity	Location
Input Sanitization		RadarStake.sol:L13-L15

Description:

The linked function accepts an address argument yet does not properly sanitize it.

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
13constructor(address radarTokenContractAddr) {
14    radarTokenContract = iRadarToken(radarTokenContractAddr);
15}

Recommendation:

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

Alleviation (8614454f02ec2934ccfdc3f17974be01ca6c1709):

The radarTokenContractAddr variable is now adequately sanitized during the contract's RadarStake::constructor, ensuring that it cannot be accidentally misconfigured during deployment.

RSE-03S: Improper Invocation of EIP-20 transferFrom

Type	Severity	Location
Standard Conformity		RadarStake.sol:L167

Description:

The linked statement does not properly validate the returned bool of the EIP-20 standard transferFrom function. 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
167radarTokenContract.transferFrom(address(this), to, 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.

Alleviation (9193d77dde992ca090fe8895a88804e096173564):

The DappRadar team has stated that the token they are interacting with is a known implementation that is conformant to the EIP-20 standard and as such the usage of a safe wrapper library is not needed. Given that the referenced invocation interacts solely with the project-affiliated Radar token, we consider this exhibit nullified as the current code is optimal and safe.