Paladin contest - Dravee's results

QA Report

1. Missing events for state-changing functions

HolyPaladinToken.setKickRatio(uint256) (contracts/HolyPaladinToken.sol#1416-1419) should emit an event for: 
	- kickRatioPerWeek = newKickRatioPerWeek (contracts/HolyPaladinToken.sol#1418) 
HolyPaladinToken.setEndDropPerSecond(uint256) (contracts/HolyPaladinToken.sol#1433-1436) should emit an event for: 
	- endDropPerSecond = newEndDropPerSecond (contracts/HolyPaladinToken.sol#1435) 

2. Immutable addresses should be 0-checked

Consider adding an address(0) check here:

HolyPaladinToken.constructor(address,address,address,uint256,uint256,uint256,uint256,uint256,uint256)._rewardsVault (contracts/HolyPaladinToken.sol#174) lacks a zero-check on :
		- rewardsVault = _rewardsVault (contracts/HolyPaladinToken.sol#194)

3. Avoid floating/different pragmas: the version should be locked in the solution before deployment:

contracts/HolyPaladinToken.sol:
  2: pragma solidity ^0.8.10;

contracts/PaladinRewardReserve.sol:
  2: pragma solidity ^0.8.4;

4. Related data should be grouped in a struct.

The User-related maps should be grouped in a struct.

From:

contracts/HolyPaladinToken.sol:
   57:     mapping(address => UserLock[]) public userLocks;
  127:     mapping(address => uint256) public userRewardIndex;
  141:     mapping(address => uint256) public userCurrentBonusRatio;
  143:     mapping(address => uint256) public userBonusRatioDecrease;

To

    struct UserInfo {
        UserLock[] locks;
        uint256 rewardIndex;
        uint256 currentBonusRatio;
        uint256 bonusRatioDecrease;
    }

    mapping(address => UserInfo) public userInfo;

It would be less error-prone, more readable, and it would be possible to delete all related fields with a simple delete userInfo[address].

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • Findings
    • Storage
      • Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it
      • Caching storage values in memory
    • Variables
      • No need to explicitly initialize variables with default values
      • Variable that should be constant
    • Comparisons
      • > 0 is less efficient than != 0 for unsigned integers (with proof)
    • Arithmetics
      • Unchecking arithmetics operations that can't underflow/overflow
      • Shift Right instead of Dividing by 2
    • Errors
      • Use Custom Errors instead of Revert Strings to save Gas

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Findings

Storage

Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.

The effect can be quite significant.

As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.

Instances include (check the @audit tags):

contracts/HolyPaladinToken.sol:
   351:         uint256 previousLockAmount = userLocks[msg.sender][currentUserLockIndex].amount;//@audit gas: should've declared "UserLock storage currentUserLock = userLocks[msg.sender][currentUserLockIndex];" and use it to access amount
   359:         if(duration == userLocks[msg.sender][currentUserLockIndex].duration) { //@audit gas: should've used suggested "UserLock storage currentUserLock = userLocks[msg.sender][currentUserLockIndex];" to access duration
   506:         if (totalLocks[nbCheckpoints - 1].fromBlock <= blockNumber) { //@audit gas: should've declared "TotalLock storage totalLock = totalLocks[nbCheckpoints - 1];" and use it to access fromBlock
   507:             return totalLocks[nbCheckpoints - 1]; //@audit gas: should've used suggested "TotalLock storage totalLock = totalLocks[nbCheckpoints - 1];" instead
   520:             if (totalLocks[mid].fromBlock == blockNumber) { //@audit gas: should've declared "TotalLock storage totalLockMid = totalLocks[mid];" and use it to cache in memory the fromBlock value
   521:                 return totalLocks[mid]; //@audit gas: should've used suggested "TotalLock storage totalLockMid = totalLocks[mid];" instead
   571:             uint256(userLocks[user][lastUserLockIndex].amount), //@audit gas: should've cached userLocks[user][lastUserLockIndex].amount in memory
   632:         uint256 lockAmount = userLocks[user][nbLocks - 1].amount;  //@audit gas: should've declared "UserLock storage userLock = userLocks[user][nbLocks - 1];" and use it to access amount
   633:         uint256 bonusVotes = delegates[user] == user && userLocks[user][nbLocks - 1].duration >= ONE_YEAR ? (lockAmount * bonusLockVoteRatio) / UNIT : 0; //@audit gas: should've used suggested "UserLock storage userLock = userLocks[user][nbLocks - 1];" and use it to access duration
   678:         if (delegateCheckpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) {  //@audit gas: should've used "DelegateCheckpoint storage delegateCheckpoint = delegateCheckpoints[account][nbCheckpoints - 1];"
   679:             return delegateCheckpoints[account][nbCheckpoints - 1].delegate; //@audit gas: should've used "DelegateCheckpoint storage delegateCheckpoint = delegateCheckpoints[account][nbCheckpoints - 1];"
   692:             if (delegateCheckpoints[account][mid].fromBlock == blockNumber) { //@audit gas: should've used "DelegateCheckpoint storage delegatecheckpointMid" and cache fromBlock
   693:                 return delegateCheckpoints[account][mid].delegate; //@audit gas: should've used "DelegateCheckpoint storage delegatecheckpointMid"
   816:                     userLockedBalance = uint256(userLocks[user][vars.lastUserLockIndex].amount);  //@audit gas: should've declared "UserLock storage userLock = userLocks[user][vars.lastUserLockIndex];" and use it to access amount
   819:                     if(userLockedBalance > 0 && userLocks[user][vars.lastUserLockIndex].duration != 0){ //@audit gas: should've used suggested "UserLock storage userLock = userLocks[user][vars.lastUserLockIndex];" and use it to access duration
   935:         if (userLocks[account][nbCheckpoints - 1].fromBlock <= blockNumber) {  //@audit gas: should've declared "UserLock storage userLock = userLocks[account][nbCheckpoints - 1];" and use it to cache in memory the fromBlock value
   936:             return userLocks[account][nbCheckpoints - 1]; //@audit gas: should've used suggested "UserLock storage userLock = userLocks[account][nbCheckpoints - 1];" instead
   949:             if (userLocks[account][mid].fromBlock == blockNumber) { //@audit gas: should've declared "UserLock storage userLockMid = userLocks[account][mid];" and use it to cache in memory the fromBlock value
   950:                 return userLocks[account][mid]; //@audit gas: should've used suggested "UserLock storage userLockMid = userLocks[account][mid];"
   969:         if (checkpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) { //@audit gas: should've used "Checkpoint storage checkpointMid"
   970:             return checkpoints[account][nbCheckpoints - 1].votes; //@audit gas: should've used "Checkpoint storage checkpointMid"
   981:             if (checkpoints[account][mid].fromBlock == blockNumber) { //@audit gas: should've used "Checkpoint storage checkpointMid" and cache fromBlock
   982:                 return checkpoints[account][mid].votes;//@audit gas: should've used "Checkpoint storage checkpointMid"
  1001:         if (delegateCheckpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) { //@audit gas: should've used "DelegateCheckpoint storage delegateCheckpoint = delegateCheckpoints[account][nbCheckpoints - 1];" and cache fromBlock
  1002:             return delegateCheckpoints[account][nbCheckpoints - 1].delegate;  //@audit gas: should've used "DelegateCheckpoint storage delegateCheckpoint = delegateCheckpoints[account][nbCheckpoints - 1];"
  1013:             if (delegateCheckpoints[account][mid].fromBlock == blockNumber) { //@audit gas: should've used "DelegateCheckpoint storage delegatecheckpointMid = delegateCheckpoints[account][mid];" and cache fromBlock
  1014:                 return delegateCheckpoints[account][mid].delegate;//@audit gas: should've used "DelegateCheckpoint storage delegatecheckpointMid = delegateCheckpoints[account][mid];"
  1051:         if (pos > 0 && checkpoints[delegatee][pos - 1].fromBlock == block.number) {  //@audit gas: should've used "Checkpoint storage checkpoint"
  1052:             checkpoints[delegatee][pos - 1].votes = safe224(newVotes);  //@audit gas: should've used "Checkpoint storage checkpoint"

This is a known practice in the same contract, as several UserLock storage currentUserLock are declared and used instead of repeatedly fetching the struct's reference.

Caching storage values in memory

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, storage values should get cached in memory (see the @audit tags for further details):

contracts/HolyPaladinToken.sol:
   520:             if (totalLocks[mid].fromBlock == blockNumber) { //@audit gas: should've declared "TotalLock storage totalLockMid = totalLocks[mid];" and use it to cache in memory the fromBlock value
   523:             if (totalLocks[mid].fromBlock > blockNumber) { //@audit gas: should've used suggested cached fromBlock value instead
   571:             uint256(userLocks[user][lastUserLockIndex].amount), //@audit gas: should've cached userLocks[user][lastUserLockIndex].amount in memory
   572:             balanceOf(user) - uint256(userLocks[user][lastUserLockIndex].amount) //@audit gas: should've used suggested cached userLocks[user][lastUserLockIndex].amount in memory
   683:         if (delegateCheckpoints[account][0].fromBlock > blockNumber) { //@audit should've used suggested cached fromBlock
   692:             if (delegateCheckpoints[account][mid].fromBlock == blockNumber) { //@audit gas: should've used "DelegateCheckpoint storage delegatecheckpointMid" and cache fromBlock
   695:             if (delegateCheckpoints[account][mid].fromBlock > blockNumber) { //@audit should've used suggested cached fromBlock
   935:         if (userLocks[account][nbCheckpoints - 1].fromBlock <= blockNumber) {  //@audit gas: should've declared "UserLock storage userLock = userLocks[account][nbCheckpoints - 1];" and use it to cache in memory the fromBlock value
   949:             if (userLocks[account][mid].fromBlock == blockNumber) { //@audit gas: should've declared "UserLock storage userLockMid = userLocks[account][mid];" and use it to cache in memory the fromBlock value
   952:             if (userLocks[account][mid].fromBlock > blockNumber) { //@audit gas: should've used the suggested cached fromBlock value
   981:             if (checkpoints[account][mid].fromBlock == blockNumber) { //@audit gas: should've used "Checkpoint storage checkpointMid" and cache fromBlock
   984:             if (checkpoints[account][mid].fromBlock > blockNumber) { //@audit gas: should've used suggested cached fromBlock
  1001:         if (delegateCheckpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) { //@audit gas: should've used "DelegateCheckpoint storage delegateCheckpoint = delegateCheckpoints[account][nbCheckpoints - 1];" and cache fromBlock
  1006:         if (delegateCheckpoints[account][0].fromBlock > blockNumber) return address(0);  //@audit gas: should've used suggested cached fromBlock
  1013:             if (delegateCheckpoints[account][mid].fromBlock == blockNumber) { //@audit gas: should've used "DelegateCheckpoint storage delegatecheckpointMid = delegateCheckpoints[account][mid];" and cache fromBlock
  1016:             if (delegateCheckpoints[account][mid].fromBlock > blockNumber) { //@audit gas: should've used suggested cached fromBlock

Variables

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Instances include:

HolyPaladinToken.sol:103:    bool public emergency = false;
HolyPaladinToken.sol:516:        uint256 low = 0;
HolyPaladinToken.sol:688:        uint256 low = 0;
HolyPaladinToken.sol:796:        uint256 userLockedBalance = 0;
HolyPaladinToken.sol:807:                uint256 lockingRewards = 0;
HolyPaladinToken.sol:945:        uint256 low = 0;
HolyPaladinToken.sol:977:        uint256 low = 0;
HolyPaladinToken.sol:1009:        uint256 low = 0;

I suggest removing explicit initializations for default values.

Variable that should be constant

This variable is never updated. It should be declared as constant:

HolyPaladinToken.bonusLockVoteRatio (contracts/HolyPaladinToken.sol#100) should be constant

Comparisons

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

HolyPaladinToken.sol:229:        require(balanceOf(msg.sender) > 0, "hPAL: No balance");
HolyPaladinToken.sol:385:        require(amount > 0, "hPAL: incorrect amount");
HolyPaladinToken.sol:1062:        require(amount > 0, "hPAL: Null amount");
HolyPaladinToken.sol:1078:        require(amount > 0, "hPAL: Null amount");
HolyPaladinToken.sol:1237:        require(userLocks[user].length > 0, "hPAL: No Lock");
HolyPaladinToken.sol:1246:        require(currentUserLock.amount > 0, "hPAL: No Lock");
HolyPaladinToken.sol:1272:        require(userLocks[user].length > 0, "hPAL: No Lock");
HolyPaladinToken.sol:1281:        require(currentUserLock.amount > 0, "hPAL: No Lock");
HolyPaladinToken.sol:1342:        require(amount > 0, "hPAL: Null amount");

Also, please enable the Optimizer.

Arithmetics

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

I suggest wrapping with an unchecked block here (see @audit tags for more details):

contracts/HolyPaladinToken.sol:
   270          require(userLocks[msg.sender].length != 0, "hPAL: No Lock");
   271          // Find the current Lock
   272:         uint256 currentUserLockIndex = userLocks[msg.sender].length - 1; //@audit gas: should be unchecked (can't underflow due to L270)

   286          require(userLocks[msg.sender].length != 0, "hPAL: No Lock");
   287          // Find the current Lock
   288:         uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;  //@audit gas: should be unchecked (can't underflow due to L286)

   348          require(userLocks[msg.sender].length != 0, "hPAL: No Lock");
   349          // Find the current Lock
   350:         uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;  //@audit gas: should be unchecked (can't underflow due to L348)

   388          uint256 claimAmount = amount < claimableRewards[msg.sender] ? amount : claimableRewards[msg.sender];
   389  
   390          // remove the claimed amount from the claimable mapping for the user, 
   391          // and transfer the PAL from the rewardsVault to the user
   392:         claimableRewards[msg.sender] -= claimAmount;  //@audit gas: should be unchecked (can't underflow due to L388)

   455          if(emergency || userLocks[user].length == 0) return UserLock(0, 0, 0, 0);
   456:         uint256 lastUserLockIndex = userLocks[user].length - 1;  //@audit gas: should be unchecked (can't underflow due to L455)

   568:         uint256 lastUserLockIndex = userLocks[user].length - 1;  //@audit gas: should be unchecked (can't underflow due to L556)

   629          if(nbLocks == 0) return currentVotes;
   630  
   631          // and if there is a lock, and user self-delegate, add the bonus voting power 
   632:         uint256 lockAmount = userLocks[user][nbLocks - 1].amount;  //@audit gas: should be unchecked (can't underflow due to L629)

   675          if (nbCheckpoints == 0) return address(0);
   676  
   677          // last checkpoint check
   678:         if (delegateCheckpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) {  //@audit gas: should be unchecked (can't underflow due to L675)
   679:             return delegateCheckpoints[account][nbCheckpoints - 1].delegate; //@audit gas: should be unchecked (can't underflow due to L675)
   
   687:         uint256 high = nbCheckpoints - 1; // last checkpoint already checked  //@audit gas: should be unchecked (can't underflow due to L675)
   
   701:         return high == 0 ? address(0) : delegateCheckpoints[account][high - 1].delegate;  //@audit gas: should be unchecked (can't underflow here)
   
   708          if(userLocks[user].length == 0) return balanceOf(user);
   709:         uint256 lastUserLockIndex = userLocks[user].length - 1;  //@audit gas: should be unchecked (can't underflow due to L708)

   727          if(block.timestamp < lastDropUpdate + MONTH) return currentDropPerSecond; // Update it once a month
   728  
   729          uint256 dropDecreasePerMonth = (startDropPerSecond - endDropPerSecond) / (dropDecreaseDuration / MONTH);
   730:         uint256 nbMonthEllapsed = (block.timestamp - lastDropUpdate) / MONTH;  //@audit gas: should be unchecked (can't underflow due to L727)

   809                  if(userLocks[user].length > 0){
   810                      UserLockRewardVars memory vars;
   811  
   812                      // and if an user has a lock, calculate the locked rewards
   813:                     vars.lastUserLockIndex = userLocks[user].length - 1;  //@audit gas: should be unchecked (can't underflow due to L809)

   828:                             newBonusRatio = vars.bonusRatioDecrease >= vars.previousBonusRatio ? 0 : vars.previousBonusRatio - vars.bonusRatioDecrease;  //@audit gas: should be unchecked (can't underflow here)

   932          if (nbCheckpoints == 0) return emptyLock;
   933  
   934          // last checkpoint check
   935:         if (userLocks[account][nbCheckpoints - 1].fromBlock <= blockNumber) {  //@audit gas: should be unchecked (can't underflow due to L932)
   936:             return userLocks[account][nbCheckpoints - 1];  //@audit gas: should be unchecked (can't underflow due to L932)

   944:         uint256 high = nbCheckpoints - 1; // last checkpoint already checked //@audit gas: should be unchecked (can't underflow due to L932)

   958:         return high == 0 ? emptyLock : userLocks[account][high - 1]; //@audit gas: should be unchecked (can't underflow here)

   966          if (nbCheckpoints == 0) return 0;
   967  
   968          // last checkpoint check
   969:         if (checkpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) { //@audit gas: should be unchecked (can't underflow due to L966)
   970:             return checkpoints[account][nbCheckpoints - 1].votes;//@audit gas: should be unchecked (can't underflow due to L966)

   976:         uint256 high = nbCheckpoints - 1; // last checkpoint already checked//@audit gas: should be unchecked (can't underflow due to L966)

   990:         return high == 0 ? 0 : checkpoints[account][high - 1].votes;//@audit gas: should be unchecked (can't underflow here)

   998          if (nbCheckpoints == 0) return address(0);
   999  
  1000          // last checkpoint check
  1001:         if (delegateCheckpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) { //@audit gas: should be unchecked (can't underflow due to L998)
  1002:             return delegateCheckpoints[account][nbCheckpoints - 1].delegate; //@audit gas: should be unchecked (can't underflow due to L998)

  1008:         uint256 high = nbCheckpoints - 1; // last checkpoint already checked //@audit gas: should be unchecked (can't underflow due to L998)

  1022:         return high == 0 ? address(0) : delegateCheckpoints[account][high - 1].delegate; //@audit gas: should be unchecked (can't underflow here)

  1030:                 uint256 oldVotes = nbCheckpoints == 0 ? 0 : checkpoints[from][nbCheckpoints - 1].votes;  //@audit gas: should be unchecked (can't underflow here)

  1039:                 uint256 oldVotes = nbCheckpoints == 0 ? 0 : checkpoints[to][nbCheckpoints - 1].votes;  //@audit gas: should be unchecked (can't underflow here)

  1051:         if (pos > 0 && checkpoints[delegatee][pos - 1].fromBlock == block.number) { //@audit gas: should be unchecked (can't underflow here)
  1052:             checkpoints[delegatee][pos - 1].votes = safe224(newVotes);  //@audit gas: should be unchecked (can't underflow here)

  1083          require(block.timestamp > (userCooldown + COOLDOWN_PERIOD), "hPAL: Insufficient cooldown");
  1084:         require(block.timestamp - (userCooldown + COOLDOWN_PERIOD) <= UNSTAKE_PERIOD, "hPAL: unstake period expired");  //@audit gas: should be unchecked (can't underflow due to L1083)
  
  1173:             uint256 currentUserLockIndex = userLocks[user].length - 1;   //@audit gas: should be unchecked (can't underflow due to L1145 and being in the else clause)

  1237          require(userLocks[user].length > 0, "hPAL: No Lock");
  1238  
  1239          // Get the user current Lock
  1240          // And calculate the end of the Lock
  1241:         uint256 currentUserLockIndex = userLocks[user].length - 1; //@audit gas: should be unchecked (can't underflow due to L1237)

  1272          require(userLocks[user].length > 0, "hPAL: No Lock");
  1273  
  1274          // Get the user to kick current Lock
  1275          // and calculate the end of the Lock
  1276:         uint256 currentUserLockIndex = userLocks[user].length - 1;  //@audit gas: should be unchecked (can't underflow due to L1272)

  1345          if(userLocks[msg.sender].length != 0){
  1346              // Check if the user has a Lock, and if so, fetch it
  1347:             uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;  //@audit gas: should be unchecked (can't underflow due to L1345)

Shift Right instead of Dividing by 2

A division by 2 can be calculated by shifting one to the right.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

I suggest replacing / 2 with >> 1 here:

HolyPaladinToken.sol:841:                            vars.periodBonusRatio = newBonusRatio + ((vars.userRatioDecrease + vars.bonusRatioDecrease) / 2);

Errors

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

HolyPaladinToken.sol:182:        require(palToken != address(0));
HolyPaladinToken.sol:183:        require(_admin != address(0));
HolyPaladinToken.sol:229:        require(balanceOf(msg.sender) > 0, "hPAL: No balance");
HolyPaladinToken.sol:270:        require(userLocks[msg.sender].length != 0, "hPAL: No Lock");
HolyPaladinToken.sol:286:        require(userLocks[msg.sender].length != 0, "hPAL: No Lock");
HolyPaladinToken.sol:301:        require(userLocks[msg.sender].length != 0, "hPAL: No Lock");
HolyPaladinToken.sol:313:        require(msg.sender != user, "hPAL: cannot kick yourself");
HolyPaladinToken.sol:348:        require(userLocks[msg.sender].length != 0, "hPAL: No Lock");
HolyPaladinToken.sol:385:        require(amount > 0, "hPAL: incorrect amount");
HolyPaladinToken.sol:493:        require(
HolyPaladinToken.sol:668:        require(
HolyPaladinToken.sol:883:            require(amount <= _availableBalanceOf(from), "hPAL: Available balance too low");
HolyPaladinToken.sol:918:        require(
HolyPaladinToken.sol:962:        require( blockNumber < block.number, "hPAL: invalid blockNumber");
HolyPaladinToken.sol:994:        require(blockNumber < block.number, "hPAL: invalid blockNumber");
HolyPaladinToken.sol:1062:        require(amount > 0, "hPAL: Null amount");
HolyPaladinToken.sol:1078:        require(amount > 0, "hPAL: Null amount");
HolyPaladinToken.sol:1079:        require(receiver != address(0), "hPAL: Address Zero");
HolyPaladinToken.sol:1083:        require(block.timestamp > (userCooldown + COOLDOWN_PERIOD), "hPAL: Insufficient cooldown");
HolyPaladinToken.sol:1084:        require(block.timestamp - (userCooldown + COOLDOWN_PERIOD) <= UNSTAKE_PERIOD, "hPAL: unstake period expired");
HolyPaladinToken.sol:1138:        require(user != address(0)); //Never supposed to happen, but security check
HolyPaladinToken.sol:1139:        require(amount != 0, "hPAL: Null amount");
HolyPaladinToken.sol:1141:        require(amount <= userBalance, "hPAL: Amount over balance");
HolyPaladinToken.sol:1142:        require(duration >= MIN_LOCK_DURATION, "hPAL: Lock duration under min");
HolyPaladinToken.sol:1143:        require(duration <= MAX_LOCK_DURATION, "hPAL: Lock duration over max");
HolyPaladinToken.sol:1194:                require(amount >=  currentUserLock.amount,"hPAL: smaller amount");
HolyPaladinToken.sol:1195:                require(duration >=  currentUserLock.duration,"hPAL: smaller duration");
HolyPaladinToken.sol:1236:        require(user != address(0)); //Never supposed to happen, but security check
HolyPaladinToken.sol:1237:        require(userLocks[user].length > 0, "hPAL: No Lock");
HolyPaladinToken.sol:1245:        require(block.timestamp > userCurrentLockEnd, "hPAL: Not expired");
HolyPaladinToken.sol:1246:        require(currentUserLock.amount > 0, "hPAL: No Lock");
HolyPaladinToken.sol:1271:        require(user != address(0) && kicker != address(0), "hPAL: Address Zero");
HolyPaladinToken.sol:1272:        require(userLocks[user].length > 0, "hPAL: No Lock");
HolyPaladinToken.sol:1280:        require(block.timestamp > userCurrentLockEnd, "hPAL: Not expired");
HolyPaladinToken.sol:1281:        require(currentUserLock.amount > 0, "hPAL: No Lock");
HolyPaladinToken.sol:1283:        require(block.timestamp > userCurrentLockEnd + UNLOCK_DELAY, "hPAL: Not kickable");
HolyPaladinToken.sol:1340:        require(emergency, "hPAL: Not emergency");
HolyPaladinToken.sol:1342:        require(amount > 0, "hPAL: Null amount");
HolyPaladinToken.sol:1343:        require(receiver != address(0), "hPAL: Address Zero");
PaladinRewardReserve.sol:29:        require(!approvedSpenders[spender], "Already Spender");
PaladinRewardReserve.sol:37:        require(approvedSpenders[spender], "Not approved Spender");
PaladinRewardReserve.sol:45:        require(approvedSpenders[spender], "Not approved Spender");

I suggest replacing revert strings with custom errors. They are already used in the solution but not extensively.