Paladin contest - TerrierLover's results

Lines of code

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

Vulnerability details

Impact

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.

Tools Used

Statis code analysis

Recommended Mitigation Steps

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

The usage of old code of openzeppelin Ownable.sol

Target codebase

Ownable.sol used in Paladin project seems to be old. https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/open-zeppelin/utils/Ownable.sol

Using the latest openzeppelin source code may be ideal.

Potential workaround

This is a latest definition of Ownable.sol. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2a4ca654046e65553af89355e24785e50735a92c/contracts/access/Ownable.sol

The notable difference is that it defines _transferOwnership internal function.

---

The usage of old version of openzeppelin Address.sol

Target codebase

Address.sol used in Paladin project is 0.8.0 which seems to be old. https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/open-zeppelin/utils/Address.sol

Address.sol in the 0.8.0 version uses assembly directly, but the compiler of solidity 0.8.1 had the improvement as mentioned in https://github.com/ethereum/solidity/releases/tag/v0.8.1.

Potential workaround

Update it to the latest Address.sol in the 0.8.2 version. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2a4ca654046e65553af89355e24785e50735a92c/contracts/utils/Address.sol

In the new version, isContract function is as follows:

function isContract(address account) internal view returns (bool) {
    // This method relies on extcodesize/address.code.length, which returns 0
    // for contracts in construction, since the code is only stored at the end
    // of the constructor execution.

    return account.code.length > 0;
}

---

Type at lock function in HolyPaladinToken.sol

Target codebase

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

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

Potential workaround

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

---

Add _ at its prefix at arguments of constructor in HolyPaladinToken.sol

Target codebase

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

constructor(
    address palToken,
    address _admin,
    address _rewardsVault,
    uint256 _startDropPerSecond,
    uint256 _endDropPerSecond,
    uint256 _dropDecreaseDuration,
    uint256 _baseLockBonusRatio,
    uint256 _minLockBonusRatio,
    uint256 _maxLockBonusRatio
){

palToken variable is the only variable without _. If there is no rationale to do so, it should add _ to align with other arguments.

Potential workaround

constructor(
    address _palToken,
    address _admin,
    address _rewardsVault,
    uint256 _startDropPerSecond,
    uint256 _endDropPerSecond,
    uint256 _dropDecreaseDuration,
    uint256 _baseLockBonusRatio,
    uint256 _minLockBonusRatio,
    uint256 _maxLockBonusRatio
){
    require(_palToken != address(0));
    require(_admin != address(0));

    pal = IERC20(_palToken);

---

No need to set 0 for uint256 variable

Target codebase

In the following codes, it initializes with 0 for uint256 variable. https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L516 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L688 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L796 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L807 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L945 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L977 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1009

Potential workaround

There is no need to initialize with 0 for uint256 variable for the above mentioned codes.

uint256 low;

---

Unnecessary parentheses are used in return statement at _getNewReceiverCooldown function in HolyPaladinToken.sol

Target codebase

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

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

Potential workaround

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

---

Unnecessary parenthesis is used in _getUserAccruedRewards function

Target codebase

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

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

Potential workaround

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

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

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

Potential improvements

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

Step1: Use the latest Ownable.sol https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2a4ca654046e65553af89355e24785e50735a92c/contracts/access/Ownable.sol

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

Deployment Gas change

---

Using the latest code of openzeppelin Ownable.sol and use _transferOwnership function at constructor of HolyPaladinToken.sol

Target codebase

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

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

constructor(
    address palToken,
    address _admin,
    address _rewardsVault,
    uint256 _startDropPerSecond,
    uint256 _endDropPerSecond,
    uint256 _dropDecreaseDuration,
    uint256 _baseLockBonusRatio,
    uint256 _minLockBonusRatio,
    uint256 _maxLockBonusRatio
){
    ...
    transferOwnership(_admin);
}

Potential improvements

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

Step1: Use the latest Ownable.sol https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2a4ca654046e65553af89355e24785e50735a92c/contracts/access/Ownable.sol

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

Deployment Gas change

---

Usage of Errors can reduce gas cost and contract size at PaladinRewardReserve.sol

Target codebase

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

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

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

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

Potential improvements

// Define error 
error AlreadySpender();
error NotApprovedSpender();

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

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

Deployment Gas change

---

No need to set false at emergency variable in HolyPaladinToken.sol

Target codebase

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

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

---

Usage of Errors can reduce gas cost and contract size at HolyPaladinToken.sol

Target codebase

Where it uses require in HolyPaladinToken.sol https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol

There are 39 callers of require.

Potential improvements

Use Errors instead of require. https://docs.soliditylang.org/en/v0.8.13/contracts.html?highlight=error#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"). https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L270 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L286 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L301 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L348 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1237 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1246 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1272 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1281

• Deployment Gas change

• Contract size change

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.

---

Use != 0 instead of > 0 in HolyPaladinToken.sol

Target codebase

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L229 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L385 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L758 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L800 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L809 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L819 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L822 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1026 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1051 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1062 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1078 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1237 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1246 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1272 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1281 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1342

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

---

Usage of unchecked can reduce the gas cost at claim function at HolyPaladinToken.sol

Target codebase

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

// 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

Deployment Gas change

---

The usage of unchecked in average function in Math.sol can reduce deployment gas fee and contract size of HolyPaladinToken.sol

Target codebase

https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/open-zeppelin/utils/Math.sol#L29

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 size change

---

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

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L526 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L698 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L955 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L987 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1019

while (low < high) {
    mid = Math.average(low, high);
    if (totalLocks[mid].fromBlock == blockNumber) {
        return totalLocks[mid];
    }
    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) {
        return totalLocks[mid];
    }
    if (totalLocks[mid].fromBlock > blockNumber) {
        high = mid;
    } else {
        unchecked {
            low = mid + 1;
        }
    }
}

Deployment Gas change

Contract size change

---

Some currentUserLockIndex varaible does not need to be defined in HolyPalatinToken.sol

Target codebase

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L272-L273 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L288-L289 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1173-L1174 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1241-L1242 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1276-L1277 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1347-L1348

// 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

Methods Gas change

Contract size change

It can reduce about 0.6% of the size of HolyPaladinToken.sol.

---

The logic to call EmergencyBlock() in emergency can be put in private function

Target codebase

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L221 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L244 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L254 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L269 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L285 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L300 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L312 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L328 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L347 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L372 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L381 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L403 https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L412

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 size change

It can reduce about 1.3% of the size of HolyPaladinToken.sol.

---

Not defining lastUserLockIndex variable decreases contract size and gas cost

Target codebase

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

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

Potential improvements

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 size change

It can reduce about 0.2% of the size of HolyPaladinToken.sol.

---

Definitions senderCooldown and receiverBalance variables are not necessary at getNewReceiverCooldown function in HolyPaladinToken.sol

Target codebase

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

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

    return _getNewReceiverCooldown(
        senderCooldown,
        amount,
        receiver,
        receiverBalance
    );
}

Definitions senderCooldown and receiverBalance variables are not necessary at getNewReceiverCooldown function in HolyPaladinToken.sol

Potential improvements

Avoid defining the above mentioned variables.

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

Deployment gas change

Contract size change

---

Not defining previousToBalance and previousFromBalance variables can reduce gas cost and contract size

Target codebase

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

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

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

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 size change

---

Avoiding calling balanceOf(user) multiple times can reduce deployment gas cost

Target codebase

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

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

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 size change

---

Avoid defining at _getNewIndex function in HolyPaladinToken.sol can reduce contract size and gas cost

Target codebase

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

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 size change

---

Avoiding defining _currentDropPerSecond and newIndex at _updateRewardState function in HolyPaladinToken.sol can reduce gas cost and contract size

Target codebase

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

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 size change

---

Not using UserLockRewardVars struct in _getUserAccruedRewards function can greatly reduces gas cost and contract size

Target codebase

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

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 size change