BadAIStaking Manual Review Findings

BAS-01M: Inexistent Clearance of Unstaking Fee Timelock

Type	Severity	Location
Logical Fault		BadAIStaking.sol:L272-L278

Description:

The consumption of an unstaking fee update via the BadAIStaking::changeUnstakingFeeRatio function will not reset the unstakingFeeRatioTimelock permitting the function to be re-invoked arbitrarily.

Impact:

Whilst the BadAIStaking::changeUnstakingFeeRatio function can be re-invoked, there is no consequence as the unstakingFeeRatio would simply be reconfigured to its existing value.

Example:

Copy
269/**
270 * @notice Change the unstaking fee ratio after the timelock has passed.
271 */
272function changeUnstakingFeeRatio() public onlyOwner {
273    require(
274        block.timestamp >= unstakingFeeRatioTimelock,
275        "BADStaking: too early to change unstaking fee."
276    );
277    unstakingFeeRatio = newUnstakingFeeRatio;
278}

Recommendation:

We advise the unstakingFeeRatioTimelock to be rest to the maximum value possible (i.e. type(uint256).max), preventing a re-invocation of the function.

Alleviation (d639d227f8b8d90dbd9813ab9d7a5cbee34dd9b1):

The unstakingFeeRatioTimelock variable is updated to the maximum value possible as advised whenever a change occurs, addressing this exhibit in full.

BAS-02M: Potentially Redundant Application of Timelock

Type	Severity	Location
Standard Conformity		BadAIStaking.sol:L256-L267

Description:

The BadAIStaking::registerNewUnstakingFeeRatio function will normally impose a unstakingFeeRatioTimelockPeriod cooldown to any unstaking fee changes, however, such a limitation should not be imposed if the fee is expected to decrease as it would be beneficial to end-users.

Impact:

This exhibit represents a potential optimization of the usability of the contract.

Example:

Copy
252/**
253 * @notice Register the intent to change the unstaking fee ratio.
254 * @param _newUnstakingFeeRatio The new unstaking fee ratio.
255 */
256function registerNewUnstakingFeeRatio(
257    uint _newUnstakingFeeRatio
258) public onlyOwner {
259    require(
260        _newUnstakingFeeRatio <= unstakingFeeDenominator,
261        "BADStaking: invalid unstaking fee."
262    );
263    newUnstakingFeeRatio = _newUnstakingFeeRatio;
264    unstakingFeeRatioTimelock =
265        block.timestamp +
266        unstakingFeeRatioTimelockPeriod;
267}
268
269/**
270 * @notice Change the unstaking fee ratio after the timelock has passed.
271 */
272function changeUnstakingFeeRatio() public onlyOwner {
273    require(
274        block.timestamp >= unstakingFeeRatioTimelock,
275        "BADStaking: too early to change unstaking fee."
276    );
277    unstakingFeeRatio = newUnstakingFeeRatio;
278}

Recommendation:

We advise fee reductions to be activated immediately as they cannot harm users and would be beneficial for all pending unstaking operations.

Alleviation (d639d227f8b8d90dbd9813ab9d7a5cbee34dd9b1):

Our recommendation has been applied, no longer requiring a timelock to pass before reducing the unstaking fee as it is considered a universally beneficial action.

BAS-03M: Inexistent Initialization of Unstaking Fee Timelock

Type	Severity	Location
Logical Fault		BadAIStaking.sol:L31, L274

Description:

The unstakingFeeRatioTimelock value is set to 0 at the creation of the contract, permitting the BadAIStaking::changeUnstakingFeeRatio function to be invoked at any time to set the unstakingFeeRatio to 0.

Impact:

The unstaking fee timelock can be bypassed during the initial configuration of the contract due to its value remaining uninitialized.

Example:

Copy
269/**
270 * @notice Change the unstaking fee ratio after the timelock has passed.
271 */
272function changeUnstakingFeeRatio() public onlyOwner {
273    require(
274        block.timestamp >= unstakingFeeRatioTimelock,
275        "BADStaking: too early to change unstaking fee."
276    );
277    unstakingFeeRatio = newUnstakingFeeRatio;
278}

Recommendation:

We advise the timelock to be initialized to the maximum value possible during the contract's initialization, ensuring that the unstaking fee ratio cannot abruptly change.

Alleviation (d639d227f8b8d90dbd9813ab9d7a5cbee34dd9b1):

The unstakingFeeRatioTimelock value is initialized to the maximum number possible when the contract's BadAIStaking::initialize function is invoked, alleviating this exhibit.

BAS-04M: Permittance of Zero-Address Stake Creation

Type	Severity	Location
Logical Fault		BadAIStaking.sol: • I-1: L115-L124 • I-2: L152-L163

Description:

The zero-address is a special flag value during BadAIStaking::updateReward operations and should possess a zero value stakes entry as otherwise BadAIStaking::showPendingReward invocations for the zero-address would yield abnormal values.

Impact:

Any stakes increase of the zero-address will result in abnormal rewards due to the userRewardPerTOkenPaid never being updated, causing BadAIStaking::showPendingReward invocations to yield incorrect values.

Example:

Copy
115function createStakeFor(
116    uint stake,
117    address to
118) public nonReentrant updateReward(to) {
119    stakingToken.safeTransferFrom(msg.sender, address(this), stake);
120    stakes[to] += stake;
121    totalStake += stake;
122
123    emit CreateStake(to, stake);
124}

Recommendation:

We advise both functions to prevent the zero-address from being specified as the to and _recipient addresses respectively, preventing inaccurate accounting of pending rewards throughout the system.

Alleviation (d639d227f8b8d90dbd9813ab9d7a5cbee34dd9b1):

Both the transfer and the creation of a stake have had a restriction introduced that disallows each respective recipient to be the zero-address, alleviating this exhibit in full.