Paladin contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

By design, a user's cooldown period is extended if they receive a transfer of hPal. The cooldown is extended based on the weight of the receiver's original balance and cooldown period compared to the sent amount and sender's cooldown period. Due to this mechanic, a malicious but generous user can indefinitely freeze the funds of another user by continuously extending the receiver's cooldown period, disabling them from unstaking.

In the most extreme case, it is possible that Pal can have negative price action in the period while the funds are frozen such that the user will be losing value despite accruing more hPal. Disregarding negative price action, having a user's funds frozen based on the authority of others would not be desired.

Proof of Concept

• User A stakes 10 Pal and receives 10 hPal
• User A calls cooldown() and waits 9 days, making his remaining cooldown period equal 1 day
• User B stakes 10 Pal, receives 10 hPal and starts their cooldown()
• User B immediately sends all 10 hPal to User A.
• User A now has a cooldown period = 5.5 days based on the following formula.

((amount * _senderCooldown) + (receiverBalance * receiverCooldown)) / (amount + receiverBalance);

With values:

((10 hPal * 10 days) + (10 hPal * 1 day)) / (10 hPal + 10 hPal) = 5.5 days

Tools Used

Manual review

Recommended Mitigation Steps

Understanding that this is a mechanism to combat users transferring funds between their own accounts to work around the cooldown system, I recommend adding an acceptTransfer() function. This function will allow the receiver to accept the sent funds and resulting new cooldown period.

Issue 1 (Low) - Missing 0 address check for immutable rewardsVault address

In the constructor for HolyPaladinToken.sol, the rewardsVault is set without requiring that the address not be 0. All other addresses in the constructor are properly verified. Moreover, rewardsVault is immutable, so the contract would need to be redeployed.

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

Issue 2 (Low) - Floating Pragma

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

Both files contain a floating pragma. It is recommended to specify a compiler version to reduce the risk of deploying contracts with different versions, potentially leading to compiler-specific bugs.

Issue 3 (Low) - Use of SafeApprove() deprecated

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

OpenZeppelin has marked this function as deprecated and recommends using safeIncreaseAllowance(). See following comments:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/742e85be7c08dff21410ba4aa9c60f6a033befb8/contracts/token/ERC20/utils/SafeERC20.sol#L39-L43

Issue 4 (Non-critical) - Make the emergency logic a modifier

Repeated emergency checks can be made a modifier to reduce code duplication.

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

Issue 1 - Redundant requires

Detail

In the following code, the second block.timestamp require statement is more exclusive than the first, making the first redundant.

Code

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