Paladin contest - IllIllI's results

Lines of code

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L692-L694

Vulnerability details

The README.md states:

If the user has a Lock, and delegates to someone, then the bonus voting power is not counted.

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/README.md?plain=1#L100

Impact

Accounts are still able to claim bonus voting power even if they delegate to someone else, and any operations that rely on the public functions getPastVotes() and getPastDelegate() for the hPAL holder account, as long as they first delegate to another account, then in a new block, delegate to themselves and then to the account to which they ultimately want to delegate. This chain of operations may or may not be intentional, leading to incorrect vote results.

Proof of Concept

Calling delegate() always adds a new entry to the delegateCheckpoints[delegator] array, regardless of the block.number:

    function _delegate(address delegator, address delegatee) internal {
        // Move delegation from the old delegate to the given delegate
        address oldDelegatee = delegates[delegator];
        uint256 delegatorBalance = balanceOf(delegator);
        delegates[delegator] = delegatee;

        // update the the Delegate chekpoint for the delegatee
        delegateCheckpoints[delegator].push(DelegateCheckpoint(safe32(block.number), delegatee));

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1317-L1324

Calling delegate() to the hPAL holder in a new block ensures that it is the first entry in the array with that block.number. Calling delegate() in the same block but to another account causes a new entry to be added, but with the same block.number.

The code that looks up whether an account has been delegated to, only checks that the block number of the entry in the array matches, and does not consider multiple entries with the same block number:

            if (delegateCheckpoints[account][mid].fromBlock == blockNumber) {
                return delegateCheckpoints[account][mid].delegate;
            }

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L692-L694

Tools Used

Code inspection

Recommended Mitigation Steps

Update existing delegateCheckpoints[delegator] if it has the same block number as the current block

Lines of code

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L875-L899

Vulnerability details

Impact

If an account has a large cooldown, that account can grief other accounts that are waiting for their own cooldowns, by sending small amounts to them.

Proof of Concept

Every transfer to an account increases the cooldown

    /** @dev Hook called before any transfer */
    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        if(from != address(0)) { //check must be skipped on minting
            // Only allow the balance that is unlocked to be transfered
            require(amount <= _availableBalanceOf(from), "hPAL: Available balance too low");
        }

        // Update user rewards before any change on their balance (staked and locked)
        _updateUserRewards(from);

        uint256 fromCooldown = cooldowns[from]; //If from is address 0x00...0, cooldown is always 0 

        if(from != to) {
            // Update user rewards before any change on their balance (staked and locked)
            _updateUserRewards(to);
            // => we don't want a self-transfer to double count new claimable rewards
            // + no need to update the cooldown on a self-transfer

            uint256 previousToBalance = balanceOf(to);
            cooldowns[to] = _getNewReceiverCooldown(fromCooldown, amount, to, previousToBalance);
        }

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L875-L899

The amount of the increase is proportional to the sender's cooldown:

        // Default new cooldown, weighted average based on the amount and the previous balance
        return ((amount * _senderCooldown) + (receiverBalance * receiverCooldown)) / (amount + receiverBalance);

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1130-L1131

Tools Used

Code inspection

Recommended Mitigation Steps

Only allow a total of one cooldown increase when the sender is not the recipient

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

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);

Refactor structs and binary searches into a common function

1. File: contracts/PaladinRewardReserve.sol (lines 944-958)

        uint256 high = nbCheckpoints - 1; // last checkpoint already checked
        uint256 low = 0;
        uint256 mid;
        while (low < high) {
            mid = Math.average(low, high);
            if (userLocks[account][mid].fromBlock == blockNumber) {
                return userLocks[account][mid];
            }
            if (userLocks[account][mid].fromBlock > blockNumber) {
                high = mid;
            } else {
                low = mid + 1;
            }
        }
        return high == 0 ? emptyLock : userLocks[account][high - 1];

2. File: contracts/PaladinRewardReserve.sol (lines 976-990)

        uint256 high = nbCheckpoints - 1; // last checkpoint already checked
        uint256 low = 0;
        uint256 mid;
        while (low < high) {
            mid = Math.average(low, high);
            if (checkpoints[account][mid].fromBlock == blockNumber) {
                return checkpoints[account][mid].votes;
            }
            if (checkpoints[account][mid].fromBlock > blockNumber) {
                high = mid;
            } else {
                low = mid + 1;
            }
        }
        return high == 0 ? 0 : checkpoints[account][high - 1].votes;

3. File: contracts/PaladinRewardReserve.sol (lines 1008-1022)

        uint256 high = nbCheckpoints - 1; // last checkpoint already checked
        uint256 low = 0;
        uint256 mid;
        while (low < high) {
            mid = Math.average(low, high);
            if (delegateCheckpoints[account][mid].fromBlock == blockNumber) {
                return delegateCheckpoints[account][mid].delegate;
            }
            if (delegateCheckpoints[account][mid].fromBlock > blockNumber) {
                high = mid;
            } else {
                low = mid + 1;
            }
        }
        return high == 0 ? address(0) : delegateCheckpoints[account][high - 1].delegate;

Use a more recent version of solidity

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

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

pragma solidity ^0.8.4;

Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27

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

    bool public emergency = false;

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

    mapping(address => bool) public approvedSpenders;

Using > 0 costs more gas than != 0 when used on a uint in a require() statement

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

        require(amount > 0, "hPAL: incorrect amount");

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

        require(amount > 0, "hPAL: Null amount");

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

        require(amount > 0, "hPAL: Null amount");

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

        require(amount > 0, "hPAL: Null amount");

It costs more gas to initialize variables to zero than to let the default of zero be applied

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

        uint256 low = 0;

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

        uint256 low = 0;

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

        uint256 userLockedBalance = 0;

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

                uint256 lockingRewards = 0;

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

        uint256 low = 0;

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

        uint256 low = 0;

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

        uint256 low = 0;

internal functions only called once can be inlined to save gas

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

    function _updateDropPerSecond() internal returns (uint256){

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

    function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256){

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

    function _unstake(address user, uint256 amount, address receiver) internal returns(uint256) {

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

    function _unlock(address user) internal {

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

    function _kick(address user, address kicker) internal {

internal functions not called by the contract should be removed to save deployment gas

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

    function _getPastDelegate(address account, uint256 blockNumber) internal view returns (address) {

State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second access of a state variable within a function. Less obvious optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, or having local caches of state variable contracts/addresses.

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

        uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;

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

        uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;

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

        uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;

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

        uint256 lastUserLockIndex = userLocks[user].length - 1;

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

        uint256 lastUserLockIndex = userLocks[user].length - 1;

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

        uint256 lockAmount = userLocks[user][nbLocks - 1].amount;

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

        uint256 lastUserLockIndex = userLocks[user].length - 1;

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

                    vars.lastUserLockIndex = userLocks[user].length - 1;

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

        if (userLocks[account][nbCheckpoints - 1].fromBlock <= blockNumber) {

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

            userLocks[user].push(UserLock(

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

        uint256 currentUserLockIndex = userLocks[user].length - 1;

12. File: contracts/HolyPaladinToken.sol (line 1276)

        uint256 currentUserLockIndex = userLocks[user].length - 1;

13. File: contracts/HolyPaladinToken.sol (line 1347)

            uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;

14. File: contracts/HolyPaladinToken.sol (line 1165)

                safe224(currentTotalLocked),

15. File: contracts/HolyPaladinToken.sol (line 1251)

            safe224(currentTotalLocked),

16. File: contracts/HolyPaladinToken.sol (line 1288)

            safe224(currentTotalLocked),

17. File: contracts/HolyPaladinToken.sol (line 1353)

                safe224(currentTotalLocked),

18. File: contracts/HolyPaladinToken.sol (line 484)

        return totalLocks[totalLocks.length - 1];

19. File: contracts/HolyPaladinToken.sol (line 506)

        if (totalLocks[nbCheckpoints - 1].fromBlock <= blockNumber) {

20. File: contracts/HolyPaladinToken.sol (line 1225)

                totalLocks.push(TotalLock(

21. File: contracts/HolyPaladinToken.sol (line 914)

        _moveDelegates(delegates[from], delegates[to], amount);

22. File: contracts/HolyPaladinToken.sol (line 624)

        uint256 currentVotes = nbCheckpoints == 0 ? 0 : checkpoints[user][nbCheckpoints - 1].votes;

23. File: contracts/HolyPaladinToken.sol (line 969)

        if (checkpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) {

24. File: contracts/HolyPaladinToken.sol (line 1030)

                uint256 oldVotes = nbCheckpoints == 0 ? 0 : checkpoints[from][nbCheckpoints - 1].votes;

25. File: contracts/HolyPaladinToken.sol (line 1051)

        if (pos > 0 && checkpoints[delegatee][pos - 1].fromBlock == block.number) {

26. File: contracts/HolyPaladinToken.sol (line 678)

        if (delegateCheckpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) {

27. File: contracts/HolyPaladinToken.sol (line 1001)

        if (delegateCheckpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) {

28. File: contracts/HolyPaladinToken.sol (line 720)

                currentDropPerSecond = endDropPerSecond;

29. File: contracts/HolyPaladinToken.sol (line 727)

        if(block.timestamp < lastDropUpdate + MONTH) return currentDropPerSecond; // Update it once a month

30. File: contracts/HolyPaladinToken.sol (line 727)

        if(block.timestamp < lastDropUpdate + MONTH) return currentDropPerSecond; // Update it once a month

31. File: contracts/HolyPaladinToken.sol (line 729)

        uint256 dropDecreasePerMonth = (startDropPerSecond - endDropPerSecond) / (dropDecreaseDuration / MONTH);

32. File: contracts/HolyPaladinToken.sol (line 388)

        uint256 claimAmount = amount < claimableRewards[msg.sender] ? amount : claimableRewards[msg.sender];

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

        uint256 estimatedClaimableRewards = claimableRewards[user];

34. File: contracts/HolyPaladinToken.sol (line 1216)

                userBonusRatioDecrease[user] = (userLockBonusRatio - baseLockBonusRatio) / duration;

35. File: contracts/HolyPaladinToken.sol (line 1157)

            uint256 userLockBonusRatio = minLockBonusRatio + (((maxLockBonusRatio - minLockBonusRatio) * durationRatio) / UNIT);

36. File: contracts/HolyPaladinToken.sol (line 1213)

                uint256 userLockBonusRatio = minLockBonusRatio + (((maxLockBonusRatio - minLockBonusRatio) * durationRatio) / UNIT);

Splitting require() statements that use && saves gas

See this issue for an example

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

        require(user != address(0) && kicker != address(0), "hPAL: Address Zero");

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

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

        uint128 amount; // safe because PAL max supply is 10M tokens

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

        uint48 startTimestamp;

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

        uint48 duration;

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

        uint32 fromBlock; // because we want to search by block number

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

        uint224 total;

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

        uint32 fromBlock;

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

        uint32 fromBlock;

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

        uint224 votes;

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

        uint32 fromBlock;

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

            uint32 blockNumber = safe32(block.number);

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

    function safe32(uint n) internal pure returns (uint32) {

12. File: contracts/HolyPaladinToken.sol (line 1392)

    function safe48(uint n) internal pure returns (uint48) {

13. File: contracts/HolyPaladinToken.sol (line 1397)

    function safe128(uint n) internal pure returns (uint128) {

14. File: contracts/HolyPaladinToken.sol (line 1402)

    function safe224(uint n) internal pure returns (uint224) {

Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code

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

    uint256 public constant WEEK = 604800;

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

    uint256 public constant MONTH = 2629800;

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

    uint256 public constant UNIT = 1e18;

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

    uint256 public constant MAX_BPS = 10000;

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

    uint256 public constant ONE_YEAR = 31557600;

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

    uint256 public constant COOLDOWN_PERIOD = 864000; // 10 days

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

    uint256 public constant UNSTAKE_PERIOD = 432000; // 5 days

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

    uint256 public constant UNLOCK_DELAY = 1209600; // 2 weeks

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

    uint256 public constant MIN_LOCK_DURATION = 7889400; // 3 months

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

    uint256 public constant MAX_LOCK_DURATION = 63115200; // 2 years

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

    uint256 public immutable startDropPerSecond;

12. File: contracts/HolyPaladinToken.sol (line 122)

    uint256 public immutable dropDecreaseDuration;

13. File: contracts/HolyPaladinToken.sol (line 124)

    uint256 public immutable startDropTimestamp;

14. File: contracts/HolyPaladinToken.sol (line 134)

    uint256 public immutable baseLockBonusRatio;

15. File: contracts/HolyPaladinToken.sol (line 136)

    uint256 public immutable minLockBonusRatio;

16. File: contracts/HolyPaladinToken.sol (line 138)

    uint256 public immutable maxLockBonusRatio;

Duplicated require()/revert() checks should be refactored to a modifier or function

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

        require(userLocks[msg.sender].length != 0, "hPAL: No Lock");

2. File: contracts/HolyPaladinToken.sol (lines 668-671)

        require(
            blockNumber < block.number,
            "hPAL: invalid blockNumber"
        );

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

        require(amount > 0, "hPAL: Null amount");

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

        require(receiver != address(0), "hPAL: Address Zero");

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

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

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

        require(userLocks[user].length > 0, "hPAL: No Lock");

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

        require(block.timestamp > userCurrentLockEnd, "hPAL: Not expired");

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

        require(currentUserLock.amount > 0, "hPAL: No Lock");

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

        require(approvedSpenders[spender], "Not approved Spender");

Multiplication/division by two should use bit shifting

<x> * 2 is equivalent to <x> << 1 and <x> / 2 is the same as <x> >> 1

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

                            vars.periodBonusRatio = newBonusRatio + ((vars.userRatioDecrease + vars.bonusRatioDecrease) / 2);

require() or revert() statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables

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

        require(msg.sender != user, "hPAL: cannot kick yourself");

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

        require(amount > 0, "hPAL: incorrect amount");

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

        require(duration >= MIN_LOCK_DURATION, "hPAL: Lock duration under min");

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

        require(duration <= MAX_LOCK_DURATION, "hPAL: Lock duration over max");

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

        require(amount > 0, "hPAL: Null amount");

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

        require(receiver != address(0), "hPAL: Address Zero");

Use custom errors rather than revert()/require() strings to save deployment gas

1. File: contracts/HolyPaladinToken.sol (Various lines throughout the file)
2. File: contracts/PaladinRewardReserve.sol (Various lines throughout the file)

Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

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

    function setKickRatio(uint256 newKickRatioPerWeek) external onlyOwner {

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

    function triggerEmergencyWithdraw(bool trigger) external onlyOwner {

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

    function setEndDropPerSecond(uint256 newEndDropPerSecond) external onlyOwner {

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

    function setNewSpender(address token, address spender, uint256 amount) external onlyOwner {

5. File: contracts/PaladinRewardReserve.sol (line 36)

    function updateSpenderAllowance(address token, address spender, uint256 amount) external onlyOwner {

6. File: contracts/PaladinRewardReserve.sol (line 44)

    function removeSpender(address token, address spender) external onlyOwner {

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

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

Not using the named return variables when a function returns, wastes deployment gas

1. File: contracts/HolyPaladinToken.sol (lines 557-561)

            return(
                balanceOf(user),
                0,
                balanceOf(user)
            );

2. File: contracts/HolyPaladinToken.sol (lines 569-573)

        return(
            balanceOf(user),
            uint256(userLocks[user][lastUserLockIndex].amount),
            balanceOf(user) - uint256(userLocks[user][lastUserLockIndex].amount)
        );

3. File: contracts/HolyPaladinToken.sol (lines 557-561)

            return(
                balanceOf(user),
                0,
                balanceOf(user)
            );

4. File: contracts/HolyPaladinToken.sol (lines 569-573)

        return(
            balanceOf(user),
            uint256(userLocks[user][lastUserLockIndex].amount),
            balanceOf(user) - uint256(userLocks[user][lastUserLockIndex].amount)
        );

5. File: contracts/HolyPaladinToken.sol (lines 557-561)

            return(
                balanceOf(user),
                0,
                balanceOf(user)
            );

6. File: contracts/HolyPaladinToken.sol (lines 569-573)

        return(
            balanceOf(user),
            uint256(userLocks[user][lastUserLockIndex].amount),
            balanceOf(user) - uint256(userLocks[user][lastUserLockIndex].amount)
        );