QA Report

[code423n4]

Low Risk Issues

PaladinRewardReserve's approvals break if the same contract is in charge of two tokens (e.g. a PalPool)

The approvedSpenders mapping only takes in a spender, rather than both a spender and a token. Approval for one token means approval for all tokens the account controls. Removal for one means removal for all.

1. File: contracts/PaladinRewardReserve.sol (lines 28-31)

   function setNewSpender(address token, address spender, uint256 amount) external onlyOwner {
       require(!approvedSpenders[spender], "Already Spender");
       approvedSpenders[spender] = true;
       IERC20(token).safeApprove(spender, amount);

2. File: contracts/PaladinRewardReserve.sol (lines 36-37)

   function updateSpenderAllowance(address token, address spender, uint256 amount) external onlyOwner {
       require(approvedSpenders[spender], "Not approved Spender");

3. File: contracts/PaladinRewardReserve.sol (lines 44-46)

   function removeSpender(address token, address spender) external onlyOwner {
       require(approvedSpenders[spender], "Not approved Spender");
       approvedSpenders[spender] = false;

Non-critical Issues

require()/revert() statements should have descriptive reason strings

1. File: contracts/HolyPaladinToken.sol (line 182)

       require(palToken != address(0));

2. File: contracts/HolyPaladinToken.sol (line 183)

       require(_admin != address(0));

3. File: contracts/HolyPaladinToken.sol (line 1138)

       require(user != address(0)); //Never supposed to happen, but security check

4. File: contracts/HolyPaladinToken.sol (line 1236)

       require(user != address(0)); //Never supposed to happen, but security check

constants should be defined rather than using magic numbers

1. File: contracts/HolyPaladinToken.sol (line 1417)

       if(newKickRatioPerWeek == 0 || newKickRatioPerWeek > 5000) revert IncorrectParameters();

The nonReentrant modifier should occur before all other modifiers

This is a best-practice to protect against reentrancy in other modifiers

1. File: contracts/PaladinRewardReserve.sol (line 52)

   function transferToken(address token, address receiver, uint256 amount) external onlyOwner nonReentrant {

safeApprove() is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance()

1. File: contracts/PaladinRewardReserve.sol (line 31)

       IERC20(token).safeApprove(spender, amount);

2. File: contracts/PaladinRewardReserve.sol (line 38)

       IERC20(token).safeApprove(spender, 0);

3. File: contracts/PaladinRewardReserve.sol (line 39)

       IERC20(token).safeApprove(spender, amount);

4. File: contracts/PaladinRewardReserve.sol (line 47)

       IERC20(token).safeApprove(spender, 0);

Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

1. File: contracts/HolyPaladinToken.sol (lines 88-94)

   mapping(address => address) public delegates;
   
   /** @notice List of Vote checkpoints for each user  */
   mapping(address => Checkpoint[]) public checkpoints;
   
   /** @notice List of Delegate checkpoints for each user  */
   mapping(address => DelegateCheckpoint[]) public delegateCheckpoints;

2. File: contracts/HolyPaladinToken.sol (lines 127-131)

   mapping(address => uint256) public userRewardIndex;
   /** @notice Current amount of rewards claimable for the user  */
   mapping(address => uint256) public claimableRewards;
   /** @notice Timestamp of last update for user rewards  */
   mapping(address => uint256) public rewardsLastUpdate;

3. File: contracts/HolyPaladinToken.sol (lines 141-143)

   mapping(address => uint256) public userCurrentBonusRatio;
   /** @notice Value by which user Bonus Ratio decrease each second  */
   mapping(address => uint256) public userBonusRatioDecrease;

Non-library/interface files should use fixed compiler versions, not floating ones

1. File: contracts/HolyPaladinToken.sol (line 2)

   pragma solidity ^0.8.10;

2. File: contracts/PaladinRewardReserve.sol (line 2)

   pragma solidity ^0.8.4;

Use the same solidity version in all non-library/interface files

1. File: contracts/HolyPaladinToken.sol (line 2)

   pragma solidity ^0.8.10;

2. File: contracts/PaladinRewardReserve.sol (line 2)

   pragma solidity ^0.8.4;

Use native time units such as seconds, minutes, hours, days, weeks and years, rather than numbers of seconds

1. File: contracts/HolyPaladinToken.sol (lines 17-39)

   uint256 public constant WEEK = 604800;
   /** @notice Seconds in a Month */
   uint256 public constant MONTH = 2629800;
   /** @notice 1e18 scale */
   uint256 public constant UNIT = 1e18;
   /** @notice Max BPS value (100%) */
   uint256 public constant MAX_BPS = 10000;
   /** @notice Seconds in a Year */
   uint256 public constant ONE_YEAR = 31557600;
   
   /** @notice  Period to wait before unstaking tokens  */
   uint256 public constant COOLDOWN_PERIOD = 864000; // 10 days
   /** @notice  Duration of the unstaking period
   After that period, unstaking cooldown is expired  */
   uint256 public constant UNSTAKE_PERIOD = 432000; // 5 days
   
   /** @notice Period to unlock/re-lock tokens without possibility of punishement   */
   uint256 public constant UNLOCK_DELAY = 1209600; // 2 weeks
   
   /** @notice Minimum duration of a Lock  */
   uint256 public constant MIN_LOCK_DURATION = 7889400; // 3 months
   /** @notice Maximum duration of a Lock  */
   uint256 public constant MAX_LOCK_DURATION = 63115200; // 2 years

Typos

1. File: contracts/HolyPaladinToken.sol (line 33)

   /** @notice Period to unlock/re-lock tokens without possibility of punishement   */

   punishement

2. File: contracts/HolyPaladinToken.sol (line 59)

   /** @notice Struct trancking the total amount locked  */

   trancking

3. File: contracts/HolyPaladinToken.sol (line 110)

   /** @notice Timstamp of last update for global reward index  */

   Timstamp

4. File: contracts/HolyPaladinToken.sol (line 113)

   /** @notice Amount of rewards distriubted per second at the start  */

   distriubted

5. File: contracts/HolyPaladinToken.sol (line 239)

    * @param amount amount ot withdraw

   ot

6. File: contracts/HolyPaladinToken.sol (line 258)

           // If the user does not deelegate currently, automatically self-delegate

   deelegate

7. File: contracts/HolyPaladinToken.sol (line 421)

    * @param receiver address fo the receiver

   fo

8. File: contracts/HolyPaladinToken.sol (line 706)

   // Find the user available balance (staked - locked) => the balance that can be transfered

   transfered

9. File: contracts/HolyPaladinToken.sol (line 802)

               // (using avaialable balance to count the locked balance with the multiplier later in this function)

   avaialable

10. File: contracts/HolyPaladinToken.sol (line 840)

                            // a ratio based on the rpevious one and the newly calculated one

    rpevious

11. File: contracts/HolyPaladinToken.sol (line 1323)

        // update the the Delegate chekpoint for the delegatee

    chekpoint

12. File: contracts/PaladinRewardReserve.sol (line 19)

    /** @notice Emitted when the allowance of a spander is updated */

    spander

Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

1. File: contracts/HolyPaladinToken.sol (line 151)

   event Stake(address indexed user, uint256 amount);

2. File: contracts/HolyPaladinToken.sol (line 153)

   event Unstake(address indexed user, uint256 amount);

3. File: contracts/HolyPaladinToken.sol (line 159)

   event Unlock(address indexed user, uint256 amount, uint256 totalLocked);

4. File: contracts/HolyPaladinToken.sol (line 161)

   event Kick(address indexed user, address indexed kicker, uint256 amount, uint256 penalty, uint256 totalLocked);

5. File: contracts/HolyPaladinToken.sol (line 163)

   event ClaimRewards(address indexed user, uint256 amount);

6. File: contracts/HolyPaladinToken.sol (line 167)

   event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance);

7. File: contracts/HolyPaladinToken.sol (line 169)

   event EmergencyUnstake(address indexed user, uint256 amount);

8. File: contracts/PaladinRewardReserve.sol (line 18)

   event NewSpender(address indexed token, address indexed spender, uint256 amount);

9. File: contracts/PaladinRewardReserve.sol (line 20)

   event UpdateSpender(address indexed token, address indexed spender, uint256 amount);