code-423n4 / 2022-03-paladin-findings

0 stars 0 forks source link

Gas Optimizations #28

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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

Contract Before After Change
PaladinRewardReserve 752599 749055 -3544

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

Contract Before After Change
PaladinRewardReserve 752599 749055 -3544

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

Contract Before After Change
PaladinRewardReserve 752599 729041 -23558

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

Contract Before After Change
HolyPaladinToken 5505231 5502829 -2402

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

Contract Before After Change
HolyPaladinToken 5505231 5498562 -6669
Contract Before After Change(KB)
HolyPaladinToken 24.018 23.987 -0.031

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.

Deployment Gas change

Contract Before After Change
HolyPaladinToken 5505231 5501226 -4005

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

Contract Method Before After Change
HolyPaladinToken claim 97548 97469 -79

Deployment Gas change

Contract Before After Change
HolyPaladinToken 5505231 5502014 -3217

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 Before After Change
HolyPaladinToken 5505231 5498742 -6489

Contract size change

Contract Before After Change(KB)
HolyPaladinToken 24.018 23.988 -0.03

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 Before After Change
HolyPaladinToken 5505231 5499198 -6033

Contract size change

Contract Before After Change(KB)
HolyPaladinToken 24.018 23.990 -0.028

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

Contract Before After Change
HolyPaladinToken 5505231 5471346 -33885

Methods Gas change

Contract Method Before After Change
HolyPaladinToken emergencyWithdraw 191020 190966 -54
HolyPaladinToken increaseLock 141196 140997 -199
HolyPaladinToken increaseLockDuration 116450 116251 -199
HolyPaladinToken kick 231670 231561 -109
HolyPaladinToken lock 336775 336764 -11
HolyPaladinToken stakeAndIncreaseLock 227758 227649 -109
HolyPaladinToken unlock 134273 134164 -109

Contract size change

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

Contract Before After Change(KB)
HolyPaladinToken 24.018 23.861 -0.157

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.

Contract Before After Change
HolyPaladinToken 5505231 5433502 -71729

Contract size change

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

Contract Before After Change(KB)
HolyPaladinToken 24.018 23.686 -0.332

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

emergencyWithdraw
increaseLock
increaseLockDuration
kick
stakeAndIncreaseLock
unlock
unstake
updateUserRewardState
Contract Before After Change
HolyPaladinToken 5505231 5433502 -71729

Contract size change

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

Contract Before After Change(KB)
HolyPaladinToken 24.018 23.959 -0.059

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 Before After Change
HolyPaladinToken 5505231 5503083 -2148

Contract size change

Contract Before After Change(KB)
HolyPaladinToken 24.018 24.008 -0.01

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

Deployment gas change

Contract Before After Change
HolyPaladinToken 5505231 5502651 -2580

Contract size change

Contract Before After Change(KB)
HolyPaladinToken 24.018 24.006 -0.012

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 Before After Change
HolyPaladinToken 5505231 5500878 -4353

Contract size change

Contract Before After Change(KB)
HolyPaladinToken 24.018 23.998 -0.02

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 Before After Change
HolyPaladinToken 5505231 5500878 -4353

Contract size change

Contract Before After Change(KB)
HolyPaladinToken 24.018 24.015 -0.002

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 Before After Change
HolyPaladinToken 5505231 5503323 -1908

Contract size change

Contract Before After Change(KB)
HolyPaladinToken 24.018 24.009 -0.009

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 Before After Change
HolyPaladinToken 5505231 5481972 -23259

Contract size change

Contract Before After Change(KB)
HolyPaladinToken 24.018 23.910 -0.108
Kogaroshi commented 2 years ago

For update of Ownable inherited contract and use of the internal transfer ownership method: https://github.com/PaladinFinance/Paladin-Tokenomics/pull/1/commits/4d0840c9c9fe8a1a4b043d2a33696fea8bf9176c

Kogaroshi commented 2 years ago

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

Kogaroshi commented 2 years ago

Really high quality Gas optimizations report