QA Report

[code423n4]

1. Events reentrancy

Risk

Low

Impact

Emitting events for some functions does not follow checks-effects-interactions pattern which leads to events reentrancy vulnerability. Attacker might perform reentrancy attack that confuses off-chain applications that monitor state of the contracts via emitted events.

Proof of Concept

HolyPaladinToken.sol

• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L394-L396
• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1070-L1072
• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1372-L1374

PaladinRewardReserve.sol

• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L31-L33
• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L39-L41
• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L47-L49

Tools Used

VS Code

Recommended Mitigation Steps

It is recommended to follow checks-effects-interactions pattern by firs emitting the event and then calling external contracts.

2. Claim rewards missing synchronization

Risk

Low

Impact

Contract HolyPaladinToken.sol uses PaladinRewardReserve.sol contract as a vault with the rewards. There is no synchronization/connection between the contracts in terms of the balances which means that HolyPaladinToken.sol might have claimableRewards[user] with the amount of rewards that should be sent to the user and PaladinRewardReserve.sol not holding enough rewards to pay the user.

Function claim is trying to send to user rewards from rewardsVault based on claimableRewards[user], but that might fail if the rewardVault does not hold enough rewards.

Proof of Concept

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

Tools Used

VS Code

Recommended Mitigation Steps

It is recommended to connect user's reward distribution with the balance of reward tokens owned by PaladinRewardReserver.sol contract.

3. Missing zero address checks

Risk

Low

Impact

Most of the functions in HolyPaladinToken.sol and PaladinRewardReserve.sol have zero address checks for setting state variables and using addresses for internal transaction logic, but there are also functions that are missing these checks. Setting some of the state variables to the zero address, whether intentional or not, can break the protocol functionality. Adding these checks consistently would prevent this scenario.

Proof of Concept

HolyPaladinToken

• Setter missing check for _rewardsVault address - https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L174
• Setter missing check for delegatee address - https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L371

PaladinRewardReserver.sol

• Setter missing check for _admin address - https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L24
• Setter missing checks for token, spender addresses - https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L28
• Missing checks for token, spender addresses - https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L36
• Missing check for token, spender addresses - https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L44
• Missing check for token, receiver addresses - https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L52

Tools Used

VS Code

Recommended Mitigation Steps

It is recommended at the minimum to add zero address checks for all listed setter functions. In addition adding checks for functions that are using user's supplied addresses could prevent some unexpected behavior of the contracts.

4. Owner - critical address change

Risk

Low

Impact

Changing critical addresses such as ownership of HolyPaladinTokens.sol and PaladinRewardReserver.sol contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L12
• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L11

Tools Used

VS Code

Recommended Mitigation Steps

It is recommended to implement two-step process for passing ownership for HolyPaladinToken.sol and PaladinRewardReserve.sol contracts.

5. Missing events

Risk

Low

Impact

Contract HolyPaladinToken.sol is missing events for triggerEmergencyWithdraw function. Lack of events makes it difficult for off-chain applications to monitor if contract is in emergency mode and if users should perform emergency withdraw.

In addition functions setKickRatio and setEndDropPerSecond are missing emitting events.

Proof of Concept

• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1425-L1427
• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1416-L1419
• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1433-L1436

Tools Used

VS Code

Recommended Mitigation Steps

Add emitting events to:

• triggerEmergencyWithdraw
• setKickRatio
• setEndDropPerSecond

6. Cooldown can be used in emergency mode

Risk

Low

Impact

Contract HolyPaladinToken.sol implements emergency mode that disables contract functionality and allows emergencyWithdraw. Function cooldown does not check if the contract is in emergency mode which allows changing contract state variable cooldowns[] even when emergency mode is enabled.

Proof of Concept

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

Tools Used

VS Code

Recommended Mitigation Steps

It is recommended to add the same check that can be found in all other functions if(emergency) revert EmergencyBlock();.

7. Inconsistent use of msg.sender and _msgSender()

Risk

Low

Impact

Contract HolyPaladinToken.sol uses _msgSender() in delegate function which is inconsistent with use of msg.sender in all the other functions. This can potentially create vulnerabilities if _msgSender() gets overridden later on and the two values are not identical anymore (e.g. if a network is paying for the transaction fees).

Proof of Concept

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

Tools Used

VS Code

Recommended Mitigation Steps

It is recommended to use msg.sender in delegate function, or switch to the usage of _msgSender() in whole contract.

8. safeApprove deprecated

Risk

Non-Critical

Impact

The OpenZeppelin SafeERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code. Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219.

Proof of Concept

• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L31
• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L38-L39
• https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/PaladinRewardReserve.sol#L47

Tools Used

VS Code

Recommended Mitigation Steps

As suggested by the OpenZeppelin comment, replace safeApprove() with safeIncreaseAllowance().

9. The contracts use unlocked pragma

Risk

Non-Critical

Impact

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced.

Proof of Concept

All the contracts in scope use unlocked pragma:

Examples:

• (^0.8.10) https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol
• (^0.8.4) https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/PaladinRewardReserve.sol

Tools Used

VS Code

Recommended Mitigation Steps

Consider locking compiler version, for example pragma solidity 0.8.10.

10. Contracts use different compiler versions

Risk

Non-Critical

Impact

Using different compiler versions across contracts of the same project might lead to confusion and accidental errors.

Proof of Concept

Examples:

• (^0.8.10) https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol
• (^0.8.4) https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/PaladinRewardReserve.sol

Tools Used

VS Code

Recommended Mitigation Steps

Consider using a single compiler version for compiling both contracts, for example 0.8.10

11. Function _getPastDelegate is never used

Risk

Non-Critical

Impact

Function _getPastDelegate is implemented but it is never used. Its existence just increase the amount of gas needed to be used for the deployment and makes contract code less readable.

Proof of Concept

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

Tools Used

Slither

Recommended Mitigation Steps

It is recommended to delete _getPastDelegate function.

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

[Kogaroshi]

2-step ownership change: PaladinFinance/Paladin-Tokenomics@70a68a0