QA Report

[code423n4]

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

        Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained.

    HolyPaladinToken.sol, 633, uint256 bonusVotes = delegates[user] == user && userLocks[user][nbLocks - 1].duration >= ONE_YEAR ? (lockAmount * bonusLockVoteRatio) / UNIT : 0; 
    HolyPaladinToken.sol, 758, uint256 ratio = currentTotalSupply > 0 ? (accruedBaseAmount * UNIT) / currentTotalSupply : 0; 
    HolyPaladinToken.sol, 652, uint256 bonusVotes = getPastDelegate(user, blockNumber) == user && pastLock.duration >= ONE_YEAR ? (pastLock.amount * bonusLockVoteRatio) / UNIT : 0; 

Title: Solidity compiler versions mismatch Severity: Low Risk

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Title: Anyone can withdraw others Severity: Low Risk

Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still 1) not the desired behavior 2) can be dangerous if the receiver is a smart contract 3) the receiver may not know someone withdraw him

    HolyPaladinToken.emergencyWithdraw
    HolyPaladinToken.triggerEmergencyWithdraw

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.

    ERC20.sol._approve owner
    ERC20.sol.allowance owner
    Ownable.sol.transferOwnership newOwner
    Ownable.sol._setOwner newOwner

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    HolyPaladinToken.sol.allBalancesOf user
    HolyPaladinToken.sol._moveDelegates from
    ERC20.sol._afterTokenTransfer from
    HolyPaladinToken.sol._getNewReceiverCooldown receiver
    HolyPaladinToken.sol._stake user
    ERC20.sol.decreaseAllowance spender

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base

Deprecated safeApprove in PaladinRewardReserve.sol line 37: IERC20(token).safeApprove(spender, 0);

Deprecated safeApprove in SafeERC20.sol line 64: _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));

Deprecated safeApprove in SafeERC20.sol line 55: _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));

Deprecated safeApprove in PaladinRewardReserve.sol line 46: IERC20(token).safeApprove(spender, 0);

Deprecated safeApprove in PaladinRewardReserve.sol line 30: IERC20(token).safeApprove(spender, amount);

Deprecated safeApprove in PaladinRewardReserve.sol line 38: IERC20(token).safeApprove(spender, amount);

Deprecated safeApprove in SafeERC20.sol line 76: _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    Ownable.sol
    AccessControl.sol

Title: Require with empty message Severity: Low Risk

The following requires are with empty messages. This is very important to add a message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: HolyPaladinToken.sol, In line 1236 with Empty Require message.
    Solidity file: HolyPaladinToken.sol, In line 183 with Empty Require message.
    Solidity file: HolyPaladinToken.sol, In line 182 with Empty Require message.
    Solidity file: HolyPaladinToken.sol, In line 1138 with Empty Require message.

Title: Named return issue Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    HolyPaladinToken.sol, _getUserAccruedRewards

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    PaladinRewardReserve.sol, updateSpenderAllowance is missing a reentrancy modifier
    PaladinRewardReserve.sol, removeSpender is missing a reentrancy modifier
    PaladinRewardReserve.sol, setNewSpender is missing a reentrancy modifier

Title: Must approve 0 first Severity: Low/Med Risk

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

approve without approving 0 first PaladinRewardReserve.sol, 30, IERC20(token).safeApprove(spender, amount);

Title: Div by 0 Severity: Medium Risk

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

    Math.sol (L40) a might be 0)
    Math.sol (L40) b might be 0)
    HolyPaladinToken.sol (L1131) receiver might be 0)
    HolyPaladinToken.sol (L1131) receiverBalance might be 0)
    HolyPaladinToken.sol (L1131) amount might be 0)

Title: Override function but with different argument location Severity: Low/Med Risk

    HolyPaladinToken.sol.constructor inherent Ownable.sol.constructor but the parameters does not match
    PaladinRewardReserve.sol.constructor inherent Ownable.sol.constructor but the parameters does not match
    PaladinRewardReserve.sol.constructor inherent ReentrancyGuard.sol.constructor but the parameters does not match
    HolyPaladinToken.sol.constructor inherent ERC20.sol.constructor but the parameters does not match

[Kogaroshi]

Title: Mult instead div in compares => In the used examples (as bonusVotes = delegates[user] == user && userLocks[user][nbLocks - 1].duration >= ONE_YEAR ? (lockAmount * bonusLockVoteRatio) / UNIT : 0;), the comparaisons are not made with a division. Comparaison: bonusVotes = delegates[user] == user && userLocks[user][nbLocks - 1].duration >= ONE_YEAR The division is made on the value to assign to the variable based on the result of the comparaison

Title: Anyone can withdraw others => Not sure I understand that issue, since in withdraw (& emergencyWithdraw), we use msg.sender as the user to withdraw fund (so other user can only withdraw for them). The only possibility it to send the withdrawn funds to another address.

Title: Not verified owner => From Ownable.sol: require(newOwner != address(0), "Ownable: new owner is the zero address");, and initialized at smart contract creation as the given address (require that admin is not address 0x0 there too). Only possibility is to renounceOwnership

Title: Must approve 0 first => In this context, this is for new Spender, that have no previous allowance from that smart contract. And in case there is a previous Spender that was removed, allowance was set to 0 when removed

Title: Div by 0 => check that amount or receiverBalance will not be 0

Rest of the entries are either acknowledged, or will be handled soon (next comment to come)

[Kogaroshi]

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