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

0 stars 0 forks source link

Gas Optimizations #61

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

1. Add payable to functions that won't receive ETH

Impact

Marking a function as payable saves gas. Functions that have the onlyOwner modifier cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas.

Proof of Concept

There are many functions that have the onlyOwner modifier and can be payable

HolyPaladinToken setKickRatio triggerEmergencyWithdraw setEndDropPerSecond

PaladinRewardReserve setNewSpender updateSpenderAllowance removeSpender transferToken

Tools Used

Manual analysis

Recommended Mitigation Steps

Add payable to these functions for gas savings

2. Bitshift for divide by 2

Impact

When multiply or dividing by a power of two, it is cheaper to bitshift than to use standard math operations.

Proof of Concept

There is a divide by 2 operation on this line https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L841

Tools Used

Manual analysis

Recommended Mitigation Steps

Bitshift right by one bit instead of dividing by 2 to save gas

3. Split up require statements instead of &&

Impact

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into a separate require statements

Proof of Concept

Only one instance of this issue was found https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L1271

Tools Used

Manual analysis

Recommended Mitigation Steps

Use separate require statements instead of concatenating with &&

4. Redundant zero initialization

Impact

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

Proof of Concept

There are several cases of this issue https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L516 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L688 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L796 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L807 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L945 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L977 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L1009

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the redundant zero initialization and declare variables to zero using uint256 low;

5. Unnecessary math

Impact

There are three return values in the function allBalancesOf from HolyPaladinToken. The third return value is derived from the first two and removing this return value saves gas. In cases where this third value is not needed, the function is wasting gas. When the third value is needed, the function that needs this value should perform the math using the first two return values.

Proof of Concept

The three return values in the function allBalancesOf https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L569-L573

The third return value balanceOf(user) - uint256(userLocks[user][lastUserLockIndex].amount) is derived from the first two values. Performing this subtraction to return a third value wastes gas when this value is not needed.

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the third value returned in the allBalancesOf function

6. Cache variable to save SLOAD

Impact

Saving a state variable in a temporary variable saves gas from SLOADs

Proof of Concept

The allBalancesOf function uses the value balanceOf(user) 4 different times. Storing balanceOf(user) in a temporary variable can save gas https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L558-L572

This is also found on line 1180, when the cached value startTimestamp can be used instead of block.timestamp https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L1180

Tools Used

Manual analysis

Recommended Mitigation Steps

Store balanceOf(user) as a temporary variable to save gas

7. Use != 0 instead of > 0

Impact

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Proof of Concept

HolyPaladinToken.sol:229: require(balanceOf(msg.sender) > 0, "hPAL: No balance"); HolyPaladinToken.sol:385: require(amount > 0, "hPAL: incorrect amount"); HolyPaladinToken.sol:758: uint256 ratio = currentTotalSupply > 0 ? (accruedBaseAmount * UNIT) / currentTotalSupply : 0; HolyPaladinToken.sol:800: if(balanceOf(user) > 0){ HolyPaladinToken.sol:809: if(userLocks[user].length > 0){ HolyPaladinToken.sol:819: if(userLockedBalance > 0 && userLocks[user][vars.lastUserLockIndex].duration != 0){ HolyPaladinToken.sol:822: if(vars.previousBonusRatio > 0){ HolyPaladinToken.sol:1026: if (from != to && amount > 0) { HolyPaladinToken.sol:1051: if (pos > 0 && checkpoints[delegatee][pos - 1].fromBlock == block.number) { HolyPaladinToken.sol:1062: require(amount > 0, "hPAL: Null amount"); HolyPaladinToken.sol:1078: require(amount > 0, "hPAL: Null amount"); HolyPaladinToken.sol:1237: require(userLocks[user].length > 0, "hPAL: No Lock"); HolyPaladinToken.sol:1246: require(currentUserLock.amount > 0, "hPAL: No Lock"); HolyPaladinToken.sol:1272: require(userLocks[user].length > 0, "hPAL: No Lock"); HolyPaladinToken.sol:1281: require(currentUserLock.amount > 0, "hPAL: No Lock"); HolyPaladinToken.sol:1342: require(amount > 0, "hPAL: Null amount");

Tools Used

grep

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

8. Use Solidity errors instead of require

Impact

Solidity errors introduced in version 0.8.4 can save gas on revert conditions https://blog.soliditylang.org/2021/04/21/custom-errors/ https://twitter.com/PatrickAlphaC/status/1505197417884528640

Proof of Concept

Some error messages are used, but many require blocks still exist in the code. A few examples are https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L270 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L385 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L668

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace require blocks with new solidity errors described in https://blog.soliditylang.org/2021/04/21/custom-errors/

9. Redundant zero address checks

Impact

The _lock and _unlock functions are internal functions. Each time _lock or _unlock is called, the user input parameter is msg.sender. This will never be the zero address, so there is no need to check if this value is zero.

Proof of Concept

There is no need to check if msg.sender is the zero address so this check can be removed to save gas https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L1138 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L1236

Tools Used

Manual analysis

Recommended Mitigation Steps

Save gas by removing redundant require checks

10. Redundant userLocks length checks

Impact

A redundant check exists where the same require statement is used twice in a row

Proof of Concept

The _unlock function checks for userLocks[user].length > 0. But _unlock is an internal function and the one time it is called, the same check is performed before _unlock is called https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L301 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L1237

Tools Used

Manual analysis

Recommended Mitigation Steps

Save gas by removing redundant require checks

11. Save gas with unchecked

Impact

Use unchecked when there is no overflow risk to save gas

Proof of Concept

This is line 828 https://github.com/code-423n4/2022-03-paladin/blob/main/contracts/HolyPaladinToken.sol#L828 newBonusRatio = vars.bonusRatioDecrease >= vars.previousBonusRatio ? 0 : vars.previousBonusRatio - vars.bonusRatioDecrease;

The entire line can be unchecked because the subtraction only happens when it cannot overflow

Tools Used

Manual analysis

Recommended Mitigation Steps

Add unchecked around math that can't overflow for gas savings

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)