# Overview

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit contest outlined in this document, C4 conducted an analysis of the Paladin smart contract system written in Solidity. The audit contest took place between March 29—April 2 2022.

## Wardens

44 Wardens contributed reports to the Paladin contest:

1. Picodes
2. Czar102
3. jayjonah8
4. hubble (ksk2345 and shri4net)
5. leastwood
6. WatchPug (jtp and ming)
7. TerrierLover
8. reassor
9. IllIllI
10. jah
11. securerodd
12. cccz
13. Jujic
14. danb
15. rayn
16. gzeon
17. 0xDjango
18. robee
19. hyh
20. JC
21. csanuragjain
22. defsec
23. sseefried
24. Dravee
25. kenta
26. 0xkatana
27. hake
28. pmerkleplant
29. minhquanym
30. 0v3rf10w
31. Funen
32. teryanarmen
33. 0xmint
34. sorrynotsorry
35. Ruhum
36. okkothejawa
37. saian
38. Tomio
39. rfa
40. antonttc
41. 0xNazgul
42. Cityscape

This contest was judged by 0xean.

Final report assembled by liveactionllama.

# Summary

The C4 analysis yielded an aggregated total of 16 unique vulnerabilities. Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity and 14 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 25 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 25 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

# Scope

The code under review can be found within the C4 Paladin contest repository, and is composed of 2 smart contracts written in the Solidity programming language and includes 1,226 lines of Solidity code.

# Severity Criteria

C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.

Vulnerabilities are divided into three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.

# High Risk Findings (2)

## [H-01] DropPerSecond is not updated homogeneously, the rewards emission can be much higher than expected in some cases

Submitted by WatchPug, also found by Czar102

function _updateDropPerSecond() internal returns (uint256){
// If no more need for monthly updates => decrease duration is over
if(block.timestamp > startDropTimestamp + dropDecreaseDuration) {
// Set the current DropPerSecond as the end value
// Plus allows to be updated if the end value is later updated
if(currentDropPerSecond != endDropPerSecond) {
currentDropPerSecond = endDropPerSecond;
lastDropUpdate = block.timestamp;
}

return endDropPerSecond;
}

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

uint256 dropDecreasePerMonth = (startDropPerSecond - endDropPerSecond) / (dropDecreaseDuration / MONTH);
uint256 nbMonthEllapsed = (block.timestamp - lastDropUpdate) / MONTH;

uint256 dropPerSecondDecrease = dropDecreasePerMonth * nbMonthEllapsed;

// We calculate the new dropPerSecond value
// We don't want to go under the endDropPerSecond
uint256 newDropPerSecond = currentDropPerSecond - dropPerSecondDecrease > endDropPerSecond ? currentDropPerSecond - dropPerSecondDecrease : endDropPerSecond;

currentDropPerSecond = newDropPerSecond;
lastDropUpdate = block.timestamp;

return newDropPerSecond;
}

When current time is lastDropUpdate + (2*MONTH-1):

nbMonthEllapsed will be round down to 1, while it’s actually 1.99 months passed, but because of precision loss, the smart contract will believe it’s only 1 month elapsed, as a result, DropPerSecond will only decrease by 1 * dropDecreasePerMonth.

In another word, due to the precision loss in calculating the number of months elapsed, for each _updateDropPerSecond() there can be a short of up to 1 * dropDecreasePerMonth for the decrease of emission rate.

At the very edge case, if all the updates happened just like the scenario above. by the end of the dropDecreaseDuration, it will drop only 12 * dropDecreasePerMonth in total, while it’s expected to be 24 * dropDecreasePerMonth.

So only half of (startDropPerSecond - endDropPerSecond) is actually decreased. And the last time updateDropPerSecond is called, DropPerSecond will suddenly drop to endDropPerSecond.

### Impact

As the DropPerSecond is not updated correctly, in most of the dropDecreaseDuration, the actual rewards emission rate is much higher than expected. As a result, the total rewards emission can be much higher than expected.

Change to:

function _updateDropPerSecond() internal returns (uint256){
// If no more need for monthly updates => decrease duration is over
if(block.timestamp > startDropTimestamp + dropDecreaseDuration) {
// Set the current DropPerSecond as the end value
// Plus allows to be updated if the end value is later updated
if(currentDropPerSecond != endDropPerSecond) {
currentDropPerSecond = endDropPerSecond;
lastDropUpdate = block.timestamp;
}

return endDropPerSecond;
}

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

uint256 dropDecreasePerMonth = (startDropPerSecond - endDropPerSecond) / (dropDecreaseDuration / MONTH);
uint256 nbMonthEllapsed = UNIT * (block.timestamp - lastDropUpdate) / MONTH;

uint256 dropPerSecondDecrease = dropDecreasePerMonth * nbMonthEllapsed / UNIT;

// We calculate the new dropPerSecond value
// We don't want to go under the endDropPerSecond
uint256 newDropPerSecond = currentDropPerSecond - dropPerSecondDecrease > endDropPerSecond ? currentDropPerSecond - dropPerSecondDecrease : endDropPerSecond;

currentDropPerSecond = newDropPerSecond;
lastDropUpdate = block.timestamp;

return newDropPerSecond;
}

Mitigation chosen is different from the Warden recommendation: since we want to keep the dropPerSecond to have a monthly decrease, we set the new lastUpdate as the previous lastUpdate + (nb_of_months_since_last_update * month_duration).

## [H-02] System could be wrapped and made useless without contract whitelisting

Submitted by Picodes

Anyone could create a contract or a contract factory “PAL Locker” with a fonction to deposit PAL tokens through a contract, lock them and delegate the voting power to the contract owner. Then, the ownership of this contract could be sold. By doing so, locked hPAL would be made liquid and transferrable again. This would eventually break the overall system of hPAL, where the idea is that you have to lock them to make them non liquid to get a boosted voting power and reward rate.

Paladin should expect this behavior to happen as we’ve seen it happening with veToken models and model implying locking features (see https://lockers.stakedao.org/ and https://www.convexfinance.com/).

This behavior could eventually be beneficial to the original DAO (ex. https://www.convexfinance.com/ for Curve and Frax), but the original DAO needs to at least be able to blacklist / whitelist such contracts and actors to ensure their interests are aligned with the protocol.

### Proof of Concept

To make locked hPAL liquid, Alice could create a contact C. Then, she can deposit hPAL through the contract, lock them and delegate voting power to herself. She can then sell or tokenize the ownership of the contract C.

Depending on if Paladin wants to be optimistic or pessimistic, implement a whitelisting / blacklisting system for contracts.

Kogaroshi (Paladin) confirmed, resolved, and commented:

Changes were made to use a Whitelist similar to the veCRV & veANGLE (changes in this PR: PaladinFinance/Paladin-Tokenomics#12).
The checker will only block for Locking, allowing smart contracts to stake and use the basic version of hPAL without locking.

# Medium Risk Findings (15)

## [M-01] HolyPaladinToken.sol uses ERC20 token with a highly unsafe pattern

Submitted by jayjonah8

In HolyPaladinToken.sol it imports ERC20.sol with some changes from the original Open Zeppelin standard. One change is that the transferFrom() function does not follow the Checks Effect and Interactions safety pattern to safely make external calls to other contracts. All checks should be handled first, then any effects/state updates, followed by the external call to prevent reentrancy attacks. Currently the transferFrom() function in ERC20.sol used by HolyPaladinToken.sol calls _transfer() first and then updates the sender allowance which is highly unsafe. The openZeppelin ER20.sol contract which is the industry standard first updates the sender allowance before calling _transfer. The external call should always be done last to avoid any double spending bugs or reentrancy attacks.

### Proof of Concept

https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html

Open Zeppelins Implementation:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol

Be sure to follow the Checks Effects and Interactions safety pattern as the transferFrom function is one of the most important functions in any protocol. Consider importing the Open Zeppelin ERC20.sol contract code directly as it is battle tested and safe code.

Kogaroshi (Paladin) confirmed, resolved, and commented:

ERC20.sol used was an older version from OpenZeppelin.
We now use the latest version of ERC20.sol that has the correct Checks Effects and Interactions safety pattern.

0xean (judge) commented:

While I agree this is an issue, I think the severity has been overstated slightly. The transferFrom() function itself does not make any external calls so there is no exposure to reentrancy issues. That being said in the future if the contract had been extended in a way that enable an external call this would prove problematic.

Since the sponsor has confirmed the issue and this does violate well known best practices and open up the codebase to future issues, I will award the medium severity.

## [M-02] Incorrect number of seconds in ONE_YEAR variable

Submitted by jayjonah8

In HolyPaladinToken.sol the ONE_YEAR variable claims that there are 31557600 seconds in a year when this is incorrect. The ONE_YEAR variable is used in the getCurrentVotes() function as well as the getPastVotes() function so it is vital that the correct time in seconds be used as it can effect users negatively.

### Proof of Concept

86,400 seconds in a day x 365 = 31_536_000

The correct number of seconds in a year is 31_536_000 so the ONE_YEAR variable should be changed to ONE_YEAR = 31_536_000

Kogaroshi (Paladin) confirmed, resolved, and commented:

An incorrect value for MONTH was used, leading to all temporal constants (YEAR, max lock time, etc …) to be incorrect.

## [M-03] Users at UNSTAKE_PERIOD can assist other users in unstaking tokens.

Submitted by cccz, also found by JC and gzeon

Consider the following scenario: Day 0: User A stakes 200 tokens and calls the cooldown function. At this time, user A’s cooldown is Day 0. Day 15: User B stakes 100 tokens, but then wants to unstake tokens. So user A said that he could assist user B in unstaking tokens, and this could be done by deploying a smart contract. In the smart contract deployed by user A, user B first needs to transfer 100 tokens to user A. In the _getNewReceiverCooldown function, _senderCooldown is Day 15 and receiverCooldown is Day 0, so the latest cooldown of user A is (100 * Day 15 + 200 * Day 0)/(100+200) = Day 5.

    function _getNewReceiverCooldown(
uint256 senderCooldown,
uint256 amount,
) internal view returns(uint256) {

// If receiver has no cooldown, no need to set a new one

uint256 minValidCooldown = block.timestamp - (COOLDOWN_PERIOD + UNSTAKE_PERIOD);

// If last receiver cooldown is expired, set it back to 0

// In case the given senderCooldown is 0 (sender has no cooldown, or minting)
uint256 _senderCooldown = senderCooldown < minValidCooldown ? block.timestamp : senderCooldown;

// If the sender cooldown is better, we keep the receiver cooldown

// Default new cooldown, weighted average based on the amount and the previous balance

}

Since User A is still at UNSTAKE_PERIOD after receiving the tokens, User A unstakes 100 tokens and sends it to User B.

After calculation, we found that when user A has a balance of X and is at the edge of UNSTAKE_PERIOD, user A can assist in unstaking the X/2 amount of tokens just staked.

### Proof of Concept

After calculation, we found that the number of tokens that users at the edge of UNSTAKEPERIOD can assist in unstaking conforms to the following equation UNSTAKEPERIOD/COOLDOWNPERIOD = UNSTAKEAMOUNT/USERBALANCE, when COOLDOWNPERIOD remains unchanged, the smaller the UNSTAKEPERIOD, the less tokens the user can assist in unstaking, so UNSTAKEPERIOD can be adjusted to alleviate this situation.

Kogaroshi (Paladin) confirmed, resolved, and commented:

Reduced the Unstake Period to 2 days to reduce the possibility of such scenario.

## [M-04] cooldown is set to 0 when the user sends all tokens to himself

Submitted by cccz, also found by hyh and Czar102

In the _beforeTokenTransfer function, cooldowns will be set to 0 when the user transfers all tokens to himself.
Consider the following scenario:
Day 0: The user stakes 100 tokens and calls the cooldown function.
Day 10: the user wanted to unstake the tokens, but accidentally transferred all the tokens to himself, which caused the cooldown to be set to 0 and the user could not unstake.

### Proof of Concept

  function _beforeTokenTransfer(
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);
// If from transfer all of its balance, reset the cooldown to 0
uint256 previousFromBalance = balanceOf(from);
if(previousFromBalance == amount && fromCooldown != 0) {
cooldowns[from] = 0;
}
}
}

Kogaroshi (Paladin) confirmed, resolved, and commented:

Applying the recommended mitigation.
An extra test for that scenario was added to the hPAL tests.

0xean (judge) commented:

Based on additional information this doesn’t permanently lock user funds, but it does unintentionally extend the duration of the lock.

See issue #88:
This effectively means that user funds be frozen for an additional period, which is the difference between his former cooldown and current timestamp. There is no reason for that and it will go against user’s expectations.

Therefore, I agree this should be a medium severity issue.

## [M-05] Past state query results are susceptible to manipulation due to multiple states with same block number

Submitted by rayn, also found by IllIllI and Czar102

4 kinds of states (UserLock, TotalLock, Checkpoint, DelegateCheckpoint) are maintained in the protocol to keep record of history. For functions that query history states, target block number is used as an index to search for the corresponding state.

However, 3 (DelegateCheckpoint, TotalLock, UserLocks) out of the 4 states are allowed to have multiple entries with same fromBlock, resulting in a one-to-many mapping between block number and history entry. This makes queried results at best imprecise, and at worst manipulatable by malicious users to present an incorrect history.

### Proof of Concept

Functions that query history states including _getPastLock, getPastTotalLock, _getPastDelegate perform a binary search through the array of history states to find entry matching queried block number. However, the searched arrays can contain multiple entries with the same fromBlock.

For example the _lock function pushes a new UserLock to userLocks[user] regardless of previous lock block number.

    function _lock(address user, uint256 amount, uint256 duration, LockAction action) internal {
require(user != address(0)); //Never supposed to happen, but security check
require(amount != 0, "hPAL: Null amount");
uint256 userBalance = balanceOf(user);
require(amount <= userBalance, "hPAL: Amount over balance");
require(duration >= MIN_LOCK_DURATION, "hPAL: Lock duration under min");
require(duration <= MAX_LOCK_DURATION, "hPAL: Lock duration over max");

if(userLocks[user].length == 0){
...
}
else {
// Get the current user Lock
uint256 currentUserLockIndex = userLocks[user].length - 1;
UserLock storage currentUserLock = userLocks[user][currentUserLockIndex];
// Calculate the end of the user current lock
uint256 userCurrentLockEnd = currentUserLock.startTimestamp + currentUserLock.duration;

uint256 startTimestamp = block.timestamp;

if(currentUserLock.amount == 0 || userCurrentLockEnd < block.timestamp) {
// User locked, and then unlocked
// or user lock expired

userLocks[user].push(UserLock(
safe128(amount),
safe48(startTimestamp),
safe48(duration),
safe32(block.number)
));
}
else {
// Update of the current Lock : increase amount or increase duration
// or renew with the same parameters, but starting at the current timestamp
require(amount >=  currentUserLock.amount,"hPAL: smaller amount");
require(duration >=  currentUserLock.duration,"hPAL: smaller duration");

// If the method is called with INCREASE_AMOUNT, then we don't change the startTimestamp of the Lock

userLocks[user].push(UserLock(
safe128(amount),
action == LockAction.INCREASE_AMOUNT ? currentUserLock.startTimestamp : safe48(startTimestamp),
safe48(duration),
safe32(block.number)
));
...
}
...
}

This makes the history searches imprecise at best. Additionally, if a user intends to shadow his past states from queries through public search functions, it is possible to control the number of entries precisely such that binsearch returns the entry he wants to show.

### Tools Used

vim, ganache-cli

Adopt the same strategy as checkpoint, and modify last entry in array instead of pushing new one if it fromBlock == block.number.

Kogaroshi (Paladin) confirmed, resolved, and commented:

0xean (judge) commented:

Agree with severity assigned in this issue. Assets are not directly compromised by this vulnerability, which would be required for 3.

## [M-06] PaladinRewardReserve.sol may have potential bugs if it uses new tokens as rewards

Submitted by TerrierLover

PaladinRewardReserve.sol may have potential bugs if it uses new tokens as rewards.

### Proof of Concept

Currently, PaladinRewardReserve.sol has following behaviors:

• mapping(address => bool) public approvedSpenders does not store the info regarding which token it targets
• setNewSpender, updateSpenderAllowance, removeSpender and transferToken functions can set token arbitrarily

Hence, some corner cases may happen as follows:

• Use TokenA at PaladinRewardReserve.sol and do operations.
• Start TokenB as rewards at PaladinRewardReserve.sol.
• All the information stored in approvedSpenders was intended for TokenA. So it is possible that following corner cases happen:

• setNewSpender function cannot set new token
• If userA is already added in approvedSpenders for TokenA, it can call updateSpenderAllowance.

Do either of followings depending on the product specification:

(1) If PAL token is only used and other token will never be used at PaladinRewardReserve.sol, stop having address token argument at setNewSpender, updateSpenderAllowance, removeSpender and transferToken functions. Instead, set token at the constructor or other ways, and limit the ability to flexibly set token from functions.

(2) If other tokens potentially will be used at PaladinRewardReserve.sol, update data structure of approvedSpenders mapping and change the logic. Firstly, it should also contain the info which token it targets such as mapping(address => address => bool). Secondly, it should rewrite the require logic at each function as follows.

require(!approvedSpenders[spender][token], "Already Spender on the specified Token");
require(approvedSpenders[spender][token], "Not approved Spender on the specified Token");

Kogaroshi (Paladin) confirmed, resolved, and commented:

## [M-07] Updating the state

Submitted by jah, also found by securerodd

In the Emergency withdraw function userCurrentBonusRatio and durationRatio aren’t update which will user clime funds with the wrong ratio.

### Proof of Concept

Set these variables to zero in the EmergencyWithdraw function.

Kogaroshi (Paladin) disagreed with High severity and commented:

Contesting the severity of the issue: even after an emergency withdraw, where the BonusRatio of the user isn’t reset back to 0, users have no way to claim extra accrued rewards, as the claim method will be blocked by the Emergency state: HolyPaladinToken.sol#L381.

0xean (judge) decreased severity to Medium and commented:

Given that the contract can be moved back out of the emergency state, I don’t think the sponsor’s assessment of this being a low risk issue is correct. I do think due to the external circumstances required for this to be achieved that it probably best qualifies as a medium risk.

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.

## [M-08] Add a timelock to PaladinRewardReserve functions

Submitted by Jujic, also found by danb

The owner of PaladinRewardReserve can approve and transfer any amount of tokens with no limits on any account. This is not good for investors. To give more trust to users: these functions should be put behind a timelock.

### Tools Used

VS Code

Add a timelock to transfer and spender approved functions.

Those 2 smart contracts will be owned by a Multisig, executing decisions based on Governance Votes in the Paladin DAO. In future evolutions of the DAO, it should have a Timelock and an on-chain Governance controlling the smart contract.

## [M-09] Function cooldown() is not protected when protocol in emergency mode

Submitted by hubble

Function cooldown() is not protected when protocol is in emergency mode.
Its behavior is not consistent with the other major functions defined.

### Impact

While other major functions like stake, unstake, lock, unlock, etc., of this contract is protected by checking for emergency flag and reverting, this function cooldown() is not checked. The impact of this is that during emergency mode, users can set immediately the cooldown() and plan for unstaking when the emergency mode is lifted and cooldown period expires. This may not be the desirable behaviour expected by the protocol.

### Proof of Concept

Contract Name : HolyPaladinToken.sol Function cooldown()

Add checking for emergency mode for this function also.

if(emergency) revert EmergencyBlock();

Kogaroshi (Paladin) confirmed, resolved, and commented:

## [M-10] UserLock information can be found during emergency mode

Submitted by hubble

When the contract is in blocked state (emergency mode), the protocol wants to return an empty UserLock info, on calling the function getUserLock. However, there is another way, by which the users can find the same information.

The below function is not protected when in emergency mode, and users can use this alternatively. Line#466 function getUserPastLock(address user, uint256 blockNumber)

### Impact

There is no loss of funds, however the intention to block information (return empty lock info) is defeated, because not all functions are protected.
There is inconsistency in implementing the emergency mode check.

### Proof of Concept

Functions getUserLock and getUserPastLock

Add checking for emergency mode for this function getUserPastLock.

if(emergency) revert EmergencyBlock();

Additional user access check can be added, so that the function returns correct value when the caller(msg.sender) is admin or owner.

Kogaroshi (Paladin) confirmed, resolved, and commented:

Instead of reverting the call, we return an empty Lock (as for getUserLock()).

## [M-11] Emergency mode enable/disable issue

Submitted by reassor

Enabling emergency mode should be one way process that sets contract(s) in emergency mode. It should be not possible to revert that process, otherwise it puts owner of the contract(s) in very privileged position. Owner can trigger emergency mode, perform emergency withdrawal operations without any restrictions and then disable emergency mode.

### Proof of Concept

It is recommended to remove bool trigger parameter from triggerEmergencyWithdraw function and set emergency to true after successfully executing function.

This Issue is acknowledged. The current emergency system will not be updated.
During deployment, the admin of the contract will be set to be the Paladin DAO multisig, and the Governance will decide on admin decision for this contract. Yet we don’t want the emergency system to totally kill the contract, but to allow users to exit if there is an issue that can be remediated.

As much as any version of this contract that will be deployed as a single signer being admin of this system could present the risk of the presented scenario, in our case we use the layer that is the multisig to prevent this type of abuse.

## [M-12] Users with large cooldowns can grief other users

Submitted by IllIllI, also found by 0xDjango and csanuragjain

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

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

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

As explained in the documentation & the comments for this method, this is required to prevent users to game the system and unstake by skipping the cooldown period.
As stated in another Issue of the same kind, this type of behavior, to have an impact on another user cooldown, would require to send an amount of token consequent compared to the receiver balance, acting as a “financial safeguard” against this type of scenario being used frequently.

For another example of this logic, see the stkAAVE system, using the same logic and the same cooldown imapct calculation on transfers.

0xean (judge) commented:

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.

I am going to side with the warden here. I do see the sponsors argument that this attack is expensive to execute, but is certainly feasible. I think this qualifies as a hypothetical attack path with stated assumptions, but external requirements. The external requirements being someone with enough malice to waste their own money to do so.

While there may not be an easy solution to solve this, it’s still a valid risk to raise and for the sponsors to (potentially) disclose to users if there is in fact no way to mitigate it without undesired side effects.

## [M-13] Users Can Bypass Emergency Restrictions on updateUserRewardState()

Submitted by leastwood

The emergencyWithdraw() function intends to withdraw their tokens regardless if they are locked up for any duration. This emergency must be triggered by the owner of the contract by calling triggerEmergencyWithdraw(). A number of functions will revert when the protocol is in an emergency state, including all stake, lock, unlock and kick actions and the updating of a user’s rewards. However, a user could bypass the restriction on _updateUserRewards() by transferring a small amount of unlocked tokens to their account. _beforeTokenTransfer() will call _updateUserRewards() on the from and to accounts. As a result, users can continue to accrue rewards while the protocol is in an emergency state and it makes sense for users to delay their emergency withdraw as they will be able to claim a higher proportion of the allocated rewards.

Consider adding a check for the boolean emergency value in _beforeTokenTransfer() to not call _updateUserRewards on any account if this value is set. Alternatively, a check could be added into the _updateUserRewards() function to return if emergency is true.

Kogaroshi (Paladin) confirmed, resolved, and commented:

(made in this PR so we can use Custom Errors directly)

## [M-14] Increasing the Lock Amount on an Expired Lock Will Cause Users to Miss Out on Rewards

Submitted by leastwood

Paladin protocol allows users to increase the amount or duration of their lock while it is stil active. Increasing the amount of an active lock should only increase the total locked amount and it shouldn’t make any changes to the associated bonus ratios as the duration remains unchanged.

However, if a user increases the lock amount on an expired lock, a new lock will be created with the duration of the previous lock and the provided non-zero amount. Because the action != LockAction.INCREASE_AMOUNT check later on in the function does not hold true, userCurrentBonusRatio will contain the last updated value from the previous lock. As a result, the user will not receive any rewards for their active lock and they will need to increase the duration of the lock to fix lock’s bonus ratio.

Consider preventing users from increasing the amount on an expired lock. This should help to mitigate this issue.

Kogaroshi (Paladin) confirmed, resolved, and commented:

(made in the PR to use the Custom Errors directly)

• added extra tests to make sure this behavior is respected

# Low Risk and Non-Critical Issues

For this contest, 25 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by IllIllI received the top score from the judge.

The following wardens also submitted reports: reassor, robee, sseefried, TerrierLover, defsec, rayn, kenta, hake, Czar102, Dravee, sorrynotsorry, pmerkleplant, gzeon, 0xDjango, Ruhum, 0v3rf10w, minhquanym, teryanarmen, jah, Funen, 0xkatana, hyh, okkothejawa, and 0xmint.

## [L-01] 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.

    function setNewSpender(address token, address spender, uint256 amount) external onlyOwner {
approvedSpenders[spender] = true;
IERC20(token).safeApprove(spender, amount);
    function updateSpenderAllowance(address token, address spender, uint256 amount) external onlyOwner {
require(approvedSpenders[spender], "Not approved Spender");
    function removeSpender(address token, address spender) external onlyOwner {
require(approvedSpenders[spender], "Not approved Spender");
approvedSpenders[spender] = false;

## [N-01] require()/revert() statements should have descriptive reason strings

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

## [N-02] constants should be defined rather than using magic numbers

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

## [N-03] The nonReentrantmodifier should occur before all other modifiers

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

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

## [N-04] safeApprove() is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance()

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

## [N-05] Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

    mapping(address => address) public delegates;

/** @notice List of Vote checkpoints for each user  */

/** @notice List of Delegate checkpoints for each user  */
mapping(address => DelegateCheckpoint[]) public delegateCheckpoints;
    mapping(address => uint256) public userRewardIndex;
/** @notice Current amount of rewards claimable for the user  */
/** @notice Timestamp of last update for user rewards  */
mapping(address => uint256) public rewardsLastUpdate;
    mapping(address => uint256) public userCurrentBonusRatio;
/** @notice Value by which user Bonus Ratio decrease each second  */
mapping(address => uint256) public userBonusRatioDecrease;

## [N-06] Non-library/interface files should use fixed compiler versions, not floating ones

pragma solidity ^0.8.10;
pragma solidity ^0.8.4;

## [N-07] Use the same solidity version in all non-library/interface files

pragma solidity ^0.8.10;
pragma solidity ^0.8.4;

## [N-08] Use native time units such as seconds, minutes, hours, days, weeks and years, rather than numbers of seconds

    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

## [N-09] Typos

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

punishement

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

trancking

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

Timstamp

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

distriubted

     * @param amount amount ot withdraw

ot

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

deelegate

     * @param receiver address fo the receiver

fo

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

transfered

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

avaialable

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

rpevious

        // update the the Delegate chekpoint for the delegatee

chekpoint

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

spander

## [N-10] Event is missing indexed fields

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

    event Stake(address indexed user, uint256 amount);
    event Unstake(address indexed user, uint256 amount);
    event Unlock(address indexed user, uint256 amount, uint256 totalLocked);
    event Kick(address indexed user, address indexed kicker, uint256 amount, uint256 penalty, uint256 totalLocked);
    event ClaimRewards(address indexed user, uint256 amount);
    event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance);
    event EmergencyUnstake(address indexed user, uint256 amount);
    event NewSpender(address indexed token, address indexed spender, uint256 amount);
    event UpdateSpender(address indexed token, address indexed spender, uint256 amount);

# Gas Optimizations

For this contest, 25 reports were submitted by wardens detailing gas optimizations. The report highlighted below by TerrierLover received the top score from the judge.

The following wardens also submitted reports: robee, IllIllI, Dravee, defsec, 0xkatana, saian, Tomio, Czar102, rfa, securerodd, gzeon, kenta, hake, antonttc, Funen, minhquanym, 0v3rf10w, 0xNazgul, pmerkleplant, Cityscape, rayn, teryanarmen, 0xmint, and 0xDjango.

## [G-01] Using the latest code of openzeppelin Ownable.sol and use _transferOwnership function at constructor of PaladinRewardReserve.sol

### Target codebase

Currently, it uses transferOwnership function at the constructor of PaladinRewardReserve.sol.

constructor(address _admin) {
}

### Potential improvements

With following steps, it can reduce the deployment gas cost.

Step1: Use the latest Ownable.sol

Step2: _transferOwnership internal function instead of transferOwnership function in PaladinRewardReserve.sol

### Deployment Gas change

Contract Before After Change

## [G-02] Using the latest code of openzeppelin Ownable.sol and use _transferOwnership function at constructor of HolyPaladinToken.sol

### Target codebase

Currently, it uses transferOwnership function at the constructor of HolyPaladinToken.sol.

constructor(
uint256 _startDropPerSecond,
uint256 _endDropPerSecond,
uint256 _dropDecreaseDuration,
uint256 _baseLockBonusRatio,
uint256 _minLockBonusRatio,
uint256 _maxLockBonusRatio
){
...
}

### Potential improvements

With following steps, it can reduce the deployment gas cost.

Step1: Use the latest Ownable.sol

Step2: _transferOwnership internal function instead of transferOwnership function in HolyPaladinToken.sol

### Deployment Gas change

Contract Before After Change

## [G-03] Usage of Errors can reduce gas cost and contract size at PaladinRewardReserve.sol

### Target codebase

It uses require but using Errors in solidity can reduce the deployment gas cost.

### Potential improvements

// Define error
error NotApprovedSpender();

// Update the logic accordingly
if (approvedSpenders[spender]) {
}

if (!approvedSpenders[spender]) {
revert NotApprovedSpender();
}

### Deployment Gas change

Contract Before After Change

## [G-04] No need to set false at emergency variable in HolyPaladinToken.sol

### Potential improvements

The default value of bool is false. So there is no need to set false at emergency variable.

bool public emergency;

### Deployment Gas change

Contract Before After Change

## [G-05] Usage of Errors can reduce gas cost and contract size at HolyPaladinToken.sol

### Target codebase

Where it uses require in HolyPaladinToken.sol.

There are 39 callers of require.

### Potential improvements

Use Errors instead of require. (Errors and the Revert Statement).

The gas and size improvements after using Errors instead of require with “hPAL: No Lock” is shown below:

// Before change
require(..., "hPAL: No Lock");

// After change
if (...) revert NoLock();

There are 8 callers of require(..., "hPAL: No Lock").

### Deployment Gas change

Contract Before After Change

### Contract size change

Contract Before After Change(KB)

Only converting the above mentioned 8 callers has -6669 deployment gas cost reduction and -0.031 size reduction. There are 31 callers of require in HolyPaladinToken.sol, using Errors instead of require may potentially reduce many gas costs and sizes.

## [G-06] Use != 0 instead of > 0 in HolyPaladinToken.sol

### Potential improvements

Use != 0 instead of > 0 at the above mentioned codes. The variable is uint, so it will not be below 0 so it can just check != 0.

require(balanceOf(msg.sender) != 0, "hPAL: No balance");

### Methods Gas change

Following methods in HolyPaladinToken.sol can reduce gas (from 5 to 20) by the above mentioned changes.

• claim function
• cooldown function
• emergencyWithdraw function
• increaseLock function
• increaseLockDuration function
• kick function
• lock function
• stake function
• stakeAndIncreaseLock function
• stakeAndLock function
• transfer function
• transferFrom function
• unlock function
• unstake function
• updateRewardState function
• updateUserRewardState function

### Deployment Gas change

Contract Before After Change

## [G-07] Usage of unchecked can reduce the gas cost at claim function at HolyPaladinToken.sol

### Target codebase

// Cannot claim more than accrued rewards, but we can use a higher amount to claim all the rewards
uint256 claimAmount = amount < claimableRewards[msg.sender] ? amount : claimableRewards[msg.sender];

// remove the claimed amount from the claimable mapping for the user,
// and transfer the PAL from the rewardsVault to the user
claimableRewards[msg.sender] -= claimAmount;

claimAmount will not be more than claimableRewards[msg.sender]. Therefore, claimableRewards[msg.sender] -= claimAmount can be wrapped by unchecked.

### Potential improvements

Wrap claimableRewards[msg.sender] -= claimAmount with unchecked.

// Cannot claim more than accrued rewards, but we can use a higher amount to claim all the rewards
uint256 claimAmount = amount < claimableRewards[msg.sender] ? amount : claimableRewards[msg.sender];

// remove the claimed amount from the claimable mapping for the user,
// and transfer the PAL from the rewardsVault to the user
unchecked {
claimableRewards[msg.sender] -= claimAmount;
}

### Method Gas change

Contract Method Before After Change

### Deployment Gas change

Contract Before After Change

## [G-08] The usage of unchecked in average function in Math.sol can reduce deployment gas fee and contract size of HolyPaladinToken.sol

### Target codebase

function average(uint256 a, uint256 b) internal pure returns (uint256) {
// (a + b) / 2 can overflow.
return (a & b) + (a ^ b) / 2;
}

The above logic would not overflow, so can use unchecked.

### Potential improvements

function average(uint256 a, uint256 b) internal pure returns (uint256) {
// (a + b) / 2 can overflow.
unchecked {
return (a & b) + (a ^ b) / 2;
}
}

### Deployment Gas change

Contract Before After Change

### Contract size change

Contract Before After Change(KB)

## [G-09] The usage of unchecked in binary search can reduce deployment gas fee and contract size of HolyPaladinToken.sol

### Target codebase

low = mid + 1 used in the binary search can be wrapped by unchecked

while (low < high) {
mid = Math.average(low, high);
if (totalLocks[mid].fromBlock == blockNumber) {
}
if (totalLocks[mid].fromBlock > blockNumber) {
high = mid;
} else {
low = mid + 1;
}
}

### Potential improvements

Wrap low = mid + 1 by unchecked.

while (low < high) {
mid = Math.average(low, high);
if (totalLocks[mid].fromBlock == blockNumber) {
}
if (totalLocks[mid].fromBlock > blockNumber) {
high = mid;
} else {
unchecked {
low = mid + 1;
}
}
}

### Deployment Gas change

Contract Before After Change

### Contract size change

Contract Before After Change(KB)

## [G-10] Some currentUserLockIndex varaible does not need to be defined in HolyPalatinToken.sol

### Target codebase

// Find the current Lock
uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;
UserLock storage currentUserLock = userLocks[msg.sender][currentUserLockIndex];

Some currentUserLockIndex varaibles is defined even though they are used once. Not defining variables can reduce gas cost and contract size.

### Potential improvements

Avoid defining currentUserLockIndex variable as follows:

// Find the current Lock
UserLock storage currentUserLock = userLocks[msg.sender][userLocks[msg.sender].length - 1];

### Deployment Gas change

Contract Before After Change

### Methods Gas change

Contract Method Before After Change

### Contract size change

Contract Before After Change(KB)

## [G-11] The logic to call EmergencyBlock() in emergency can be put in private function

### Target codebase

Following check is called by 13 callsites. This can be put into the private function to reduce the contract size and deployment gas cost.

if(emergency) revert EmergencyBlock();

### Potential improvements

Define a private function to include the above mentioned logic, and use isEmergent function at each callsite.

function isEmergent() private view {
if(emergency) revert EmergencyBlock();
}

### Gas change

Please note that the method gas fee increases while deployment gas fee reduces. So it depends on what this project prioritizes.

• Method gas fee

Some methods have 10 ~ 30 increase of method gas cost.

• Deployment gas fee
Contract Before After Change

### Contract size change

Contract Before After Change(KB)

## [G-12] Not defining lastUserLockIndex variable decreases contract size and gas cost

### Target codebase

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

### Gas change

• Method gas fee

Following methods in HolyPaladinToken.sol have reduced 20~200 gas cost

emergencyWithdraw
increaseLock
increaseLockDuration
kick
stakeAndIncreaseLock
unlock
unstake
updateUserRewardState
• Deployment gas fee
Contract Before After Change

### Contract size change

Contract Before After Change(KB)

### Target codebase

function getNewReceiverCooldown(address sender, address receiver, uint256 amount) external view returns(uint256) {
uint256 senderCooldown = cooldowns[sender];

senderCooldown,
amount,
);
}

### Potential improvements

Avoid defining the above mentioned variables.

function getNewReceiverCooldown(address sender, address receiver, uint256 amount) external view returns(uint256) {
cooldowns[sender],
amount,
);
}

### Deployment gas change

Contract Before After Change

### Contract size change

Contract Before After Change(KB)

## [G-14] Not defining previousToBalance and previousFromBalance variables can reduce gas cost and contract size

### Target codebase

uint256 previousToBalance = balanceOf(to);
cooldowns[to] = _getNewReceiverCooldown(fromCooldown, amount, to, previousToBalance);
uint256 previousFromBalance = balanceOf(from);
if(previousFromBalance == amount && fromCooldown != 0) {

### Potential improvements

Avoid defining previousToBalance and previousFromBalance.

cooldowns[to] = _getNewReceiverCooldown(fromCooldown, amount, to, balanceOf(to));
if(balanceOf(from) == amount && fromCooldown != 0) {

### Method gas change

Following methods in HolyPaladinToken.sol can reduce around 10~20 gas cost

• emergencyWithdraw
• kick
• stake
• stakeAndIncreaseLock
• transferFrom
• unstake

### Deployment gas change

Contract Before After Change

### Contract size change

Contract Before After Change(KB)

## [G-15] Avoiding calling balanceOf(user) multiple times can reduce deployment gas cost

### Potential improvements

uint256 balance = balanceOf(user);
// If the contract was blocked (emergency mode) or
// If the user has no Lock
// then available == staked
if(emergency || userLocks[user].length == 0) {
return(
balance,
0,
balance
);
}
// If a Lock exists
// Then return
// total staked balance
// locked balance
// available balance (staked - locked)
uint256 lastUserLockIndex = userLocks[user].length - 1;
return(
balance,
uint256(userLocks[user][lastUserLockIndex].amount),
balance - uint256(userLocks[user][lastUserLockIndex].amount)
);

### Deployment gas change

Contract Before After Change

### Contract size change

Contract Before After Change(KB)

## [G-16] Avoid defining at _getNewIndex function in HolyPaladinToken.sol can reduce contract size and gas cost

### Target codebase

ellapsedTime variable does not need to be defined.

uint256 ellapsedTime = block.timestamp - lastRewardUpdate;
...
uint256 accruedBaseAmount = ellapsedTime * baseDropPerSecond;

### Potential improvements

Avoid defining ellapsedTime variable.

uint256 accruedBaseAmount = (block.timestamp - lastRewardUpdate) * baseDropPerSecond;

### Method gas change

This change reduces the method gas cost of more than 10 functions. The reductions are around 10~20.

### Deployment gas change

Contract Before After Change

### Contract size change

Contract Before After Change(KB)

## [G-17] Avoiding defining _currentDropPerSecond and newIndex at _updateRewardState function in HolyPaladinToken.sol can reduce gas cost and contract size

### Target codebase

uint256 _currentDropPerSecond = _updateDropPerSecond();

// Update the index
uint256 newIndex = _getNewIndex(_currentDropPerSecond);
rewardIndex = newIndex;
lastRewardUpdate = block.timestamp;

return newIndex;

### Potential improvements

// Update the index
rewardIndex = _getNewIndex(_updateDropPerSecond());
lastRewardUpdate = block.timestamp;

return rewardIndex;

### Method gas change

This change reduces the method gas cost of more than 10 functions. The reductions are around 10~20.

### Deployment gas change

Contract Before After Change

### Contract size change

Contract Before After Change(KB)

## [G-18] Not using UserLockRewardVars struct in _getUserAccruedRewards function can greatly reduces gas cost and contract size

### Target codebase

if(balanceOf(user) > 0){
// calculate the base rewards for the user staked balance
// (using avaialable balance to count the locked balance with the multiplier later in this function)
uint256 indexDiff = currentRewardsIndex - userLastIndex;

uint256 stakingRewards = (userStakedBalance * indexDiff) / UNIT;

uint256 lockingRewards = 0;

if(userLocks[user].length > 0){
UserLockRewardVars memory vars;

// and if an user has a lock, calculate the locked rewards
vars.lastUserLockIndex = userLocks[user].length - 1;

// using the locked balance, and the lock duration
userLockedBalance = uint256(userLocks[user][vars.lastUserLockIndex].amount);

// Check that the user's Lock is not empty
if(userLockedBalance > 0 && userLocks[user][vars.lastUserLockIndex].duration != 0){
vars.previousBonusRatio = userCurrentBonusRatio[user];

if(vars.previousBonusRatio > 0){
vars.userRatioDecrease = userBonusRatioDecrease[user];
// Find the new multiplier for user:
// From the last Ratio, where we remove userBonusRatioDecrease for each second since last update
vars.bonusRatioDecrease = (block.timestamp - rewardsLastUpdate[user]) * vars.userRatioDecrease;

newBonusRatio = vars.bonusRatioDecrease >= vars.previousBonusRatio ? 0 : vars.previousBonusRatio - vars.bonusRatioDecrease;

if(vars.bonusRatioDecrease >= vars.previousBonusRatio){
// Since the last update, bonus ratio decrease under 0
// We count the bonusRatioDecrease as the difference between the last Bonus Ratio and 0
vars.bonusRatioDecrease = vars.previousBonusRatio;
// In the case this update is made far after the end of the lock, this method would mean
// the user could get a multiplier for longer than expected
// We count on the Kick logic to avoid that scenario
}

// and calculate the locking rewards based on the locked balance &
// a ratio based on the rpevious one and the newly calculated one
vars.periodBonusRatio = newBonusRatio + ((vars.userRatioDecrease + vars.bonusRatioDecrease) / 2);
lockingRewards = (userLockedBalance * ((indexDiff * vars.periodBonusRatio) / UNIT)) / UNIT;
}
}

}
// sum up the accrued rewards, and return it
accruedRewards = stakingRewards + lockingRewards;
}

UserLockRewardVars struct does not need to be used.

### Potential improvements

Here is an example codebase which avoids using UserLockRewardVars memory vars.

if(balanceOf(user) > 0){
// calculate the base rewards for the user staked balance
// (using avaialable balance to count the locked balance with the multiplier later in this function)
uint256 indexDiff = currentRewardsIndex - userLastIndex;

uint256 lockingRewards = 0;

if(userLocks[user].length > 0){
// and if an user has a lock, calculate the locked rewards
uint256 lastUserLockIndex = userLocks[user].length - 1;

// using the locked balance, and the lock duration
userLockedBalance = uint256(userLocks[user][lastUserLockIndex].amount);

// Check that the user's Lock is not empty
if(userLockedBalance > 0 && userLocks[user][lastUserLockIndex].duration != 0){
uint256 previousBonusRatio = userCurrentBonusRatio[user];

if(previousBonusRatio > 0){
uint256 userRatioDecrease = userBonusRatioDecrease[user];
// Find the new multiplier for user:
// From the last Ratio, where we remove userBonusRatioDecrease for each second since last update
uint256 bonusRatioDecrease = (block.timestamp - rewardsLastUpdate[user]) * userRatioDecrease;

newBonusRatio = bonusRatioDecrease >= previousBonusRatio ? 0 : previousBonusRatio - bonusRatioDecrease;

if(bonusRatioDecrease >= previousBonusRatio){
// Since the last update, bonus ratio decrease under 0
// We count the bonusRatioDecrease as the difference between the last Bonus Ratio and 0
bonusRatioDecrease = previousBonusRatio;
// In the case this update is made far after the end of the lock, this method would mean
// the user could get a multiplier for longer than expected
// We count on the Kick logic to avoid that scenario
}

// and calculate the locking rewards based on the locked balance &
// a ratio based on the rpevious one and the newly calculated one
uint256 periodBonusRatio = newBonusRatio + ((userRatioDecrease + bonusRatioDecrease) / 2);
lockingRewards = (userLockedBalance * ((indexDiff * periodBonusRatio) / UNIT)) / UNIT;
}
}

}
// sum up the accrued rewards, and return it
accruedRewards = (userStakedBalance * indexDiff) / UNIT + lockingRewards;
}

In this example, it also does not define userStakedBalance variable.

### Method gas change

This change reduces the method gas cost of more than 10 functions. The reductions are around 100~200.

### Deployment gas change

Contract Before After Change

### Contract size change

Contract Before After Change(KB)

For update of Ownable inherited contract and use of the internal transfer ownership method: PaladinFinance/Paladin-Tokenomics@4d0840c.

(some changes/tips were implemented, others are noted but won’t be applied)

Really high quality Gas optimizations report.

# Disclosures

C4 is an open organization governed by participants in the community.

C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.