QA Report

[code423n4]

QA Report

Remarks/Recommendations

• The test suite was comprehensive and easy to run! There could have been some more tests checking what the values of rewards that are accrued.

• Functions lock, increaseLockDuration and increaseLock all relied on a call to _updateUserRewards to work properly. Consider the use of a function modifier.

• A function modifier could also be used for those functions which begin with if (emergency) revert EmergencyBlock(). For example:

modifier notEmergency {
  if(emergency) revert EmergencyBlock();
  _;
}

And then the function signature of e.g. increaseLockDuration changes like so:

function increaseLockDuration(uint256 duration) external notEmergency {

Incidentally, you can test for reversions like this in your test suite with a line like:

await hPAL.connect(admin).triggerEmergencyWithdraw(true)
await expect(hPAL.connect(user1).increaseLockDuration(63115200)).
        to.be.revertedWith("VM Exception while processing transaction: reverted with custom error 'EmergencyBlock()");

• A lot of the math involving fractional values is difficult to tell immediately whether it's correct. I suggest developing (or using) a math library for fractional values so that all the * UNIT or / UNIT parts of the expressions are removed. It would be similar to Open Zeppelin's SafeMath library but for fractional values instead.

• Days and weeks really are a fixed number of seconds (86400 and 604800 respectively). However, MONTH and ONE_YEAR are not actually 2629800 s = 30.4375 days and 31557600 s = 365.25 days respectively. One month from 1 February is only 28 days, while from 1 March it is 31 days. It's context dependent. The same goes for years. If someone locks for a year they probably don't expect to wait an extra quarter of a day.

  I suggest talking about lock durations strictly in terms of days or weeks. It will also be less confusing for users. e.g. The minimum lock period is 90 days the maximum lock period is 731 days.

• Function triggerEmergencyWithdraw has a deceptive name. Just call it setEmergency since it can also be used to set the emergency flag to false.

Low: Lack of validation on constructor parameters

Impact

If the HolyPaladinToken contract is initialised with certain (strange) values this causes many functions to revert for the lifetime of the contract. This happens because of underflow bugs -- which are checked for by Solidity 0.8.0 and upwards) -- on the following lines:

• line 1160 and line 1216 because baseLockBonusRatio > minLockBonusRatio
• line 729 because startDropPerSecond < endDropPerSecond
• line 1157 and line 1213 because minLockBonusRatio > maxLockBonusRatio

Proof of Concept

This PoC is executable. Three different bugs that lead to reversion are recreated using your testing framework in a fork of your repo:

• Bug when baseLockBonusRatio > minLockBonusRatio.
• Bug when startDropPerSecond < endDropPerSecond
• Bug when minLockBonusRatio > maxLockBonusRatio

Tools Used

Manual inspection

Recommended Mitigation Steps

Add the following require declarations to the constructor of HolyPaladinToken

require(_baseLockBonusRatio <= _minLockBonusRatio, "_baseLockBonusRatio > _minLockBonusRatio");
require(_startDropPerSecond >= _endDropPerSecond, "_startDropPerSecond < _endDropPerSecond");
require(_minLockBonusRatio <= _maxLockBonusRatio, "_minLockBonusRatio > _maxLockBonusRatio");

Non-critical: setEndDropPerSecond can be called by admin with value higher than startDropPerSecond

Impact

Once block.timestamp >= startDropTimestamp + dropDecreaseDuration it becomes possible for the admin to call setEndDropPerSecond with a value higher than
startDropPerSecond. This leads to a reversion in the exceedingly rare case that _updateDropPerSecond is called in the same transaction precisely when
block.timestamp == startDropTimestamp + dropDecreaseDuration.

This is because on line 716 of HolyPaladinToken.sol we have:

if(block.timestamp > startDropTimestamp + dropDecreaseDuration) {

which would not be true and thus the return on line 724 would not be called.

This leads to a revert caused by underflow on line 729

This is an exceedingly rare bug and unlikely to happen because

• it is the admin that calls function setEndDropPerSecond.
• it can only occur when block.timestamp == startDropTimestamp + dropDecreaseDuration. Reverts will not occur at any subsequent timestamp.

Proof of Concept

Nevertheless, I have constructed a proof of concept of this bug by adding a
new function in my fork of the repo here and writing a test which exercises the bug here.

Recommended Mitigation Steps

Add the following require declaration to function setEndDropPerSecond

require(newEndDropPerSecond <= startDropPerSecond, "newEndDropPerSecond > startDropPerSecond");

Low: Don't use safeApprove in PaladinRewardReserve contract

Impact

There are multiple uses of safeApprove in PaladinRewardReserve.sol. It has similar issues to approve where careful transaction ordering by an attacker could lead to uses of the old and the new allowance.

OpenZeppelin have deprecated this function here. A deeper discussion of the issue can be found in this GitHub Issue.

Proof of Concept

Uses of safeApprove appear in function setNewSpender, updateSpenderAllowance and removeSpender.

However, it is only its use in function updateSpenderAllowance where it is problematic.

Tools Used

Manual inspection

Recommended Mitigation Steps

The general advice given by Open Zeppelin is to use calls to safeIncreaseAllowance or safeDecreaseAllowance instead. In the case of function updateSpenderAllowance one would need to determine whether the allowance was being increased or decreased by first calling IERC20's allowance function and comparing the result to the amount parameter

Non-critical: typos

• line 59. "tranking" -> "tracking"
• line 802. "avaialable" -> "available"
• line 1081. "Check if user in inside the allowed period base on its cooldown" -> "Check if user is inside the allowed period based on its cooldown"
• line 1323.
  "chekpoint" -> "checkpoint"

[Kogaroshi]

QA & gas optimizations changes are done in the PR: PaladinFinance/Paladin-Tokenomics#6
(some changes/tips were implemented, others are noted but won't be applied)