search

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

0 stars 0 forks source link

Gas Optimizations #73

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Refactor structs and binary searches into a common function

  1. File: contracts/PaladinRewardReserve.sol (lines 944-958) 
        uint256 high = nbCheckpoints - 1; // last checkpoint already checked
    uint256 low = 0;
    uint256 mid;
    while (low < high) {
        mid = Math.average(low, high);
        if (userLocks[account][mid].fromBlock == blockNumber) {
            return userLocks[account][mid];
        }
        if (userLocks[account][mid].fromBlock > blockNumber) {
            high = mid;
        } else {
            low = mid + 1;
        }
    }
    return high == 0 ? emptyLock : userLocks[account][high - 1];
  2. File: contracts/PaladinRewardReserve.sol (lines 976-990) 
        uint256 high = nbCheckpoints - 1; // last checkpoint already checked
    uint256 low = 0;
    uint256 mid;
    while (low < high) {
        mid = Math.average(low, high);
        if (checkpoints[account][mid].fromBlock == blockNumber) {
            return checkpoints[account][mid].votes;
        }
        if (checkpoints[account][mid].fromBlock > blockNumber) {
            high = mid;
        } else {
            low = mid + 1;
        }
    }
    return high == 0 ? 0 : checkpoints[account][high - 1].votes;
  3. File: contracts/PaladinRewardReserve.sol (lines 1008-1022) 
        uint256 high = nbCheckpoints - 1; // last checkpoint already checked
    uint256 low = 0;
    uint256 mid;
    while (low < high) {
        mid = Math.average(low, high);
        if (delegateCheckpoints[account][mid].fromBlock == blockNumber) {
            return delegateCheckpoints[account][mid].delegate;
        }
        if (delegateCheckpoints[account][mid].fromBlock > blockNumber) {
            high = mid;
        } else {
            low = mid + 1;
        }
    }
    return high == 0 ? address(0) : delegateCheckpoints[account][high - 1].delegate;

Use a more recent version of solidity

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

  1. File: contracts/PaladinRewardReserve.sol (line 2) 
    pragma solidity ^0.8.4;

Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27

  1. File: contracts/HolyPaladinToken.sol (line 103) 
    bool public emergency = false;
  2. File: contracts/PaladinRewardReserve.sol (line 15) 
    mapping(address => bool) public approvedSpenders;

Using > 0 costs more gas than != 0 when used on a uint in a require() statement

  1. File: contracts/HolyPaladinToken.sol (line 385) 
        require(amount > 0, "hPAL: incorrect amount");
  2. File: contracts/HolyPaladinToken.sol (line 1062) 
        require(amount > 0, "hPAL: Null amount");
  3. File: contracts/HolyPaladinToken.sol (line 1078) 
        require(amount > 0, "hPAL: Null amount");
  4. File: contracts/HolyPaladinToken.sol (line 1342) 
        require(amount > 0, "hPAL: Null amount");

It costs more gas to initialize variables to zero than to let the default of zero be applied

  1. File: contracts/HolyPaladinToken.sol (line 516) 
        uint256 low = 0;
  2. File: contracts/HolyPaladinToken.sol (line 688) 
        uint256 low = 0;
  3. File: contracts/HolyPaladinToken.sol (line 796) 
        uint256 userLockedBalance = 0;
  4. File: contracts/HolyPaladinToken.sol (line 807) 
                uint256 lockingRewards = 0;
  5. File: contracts/HolyPaladinToken.sol (line 945) 
        uint256 low = 0;
  6. File: contracts/HolyPaladinToken.sol (line 977) 
        uint256 low = 0;
  7. File: contracts/HolyPaladinToken.sol (line 1009) 
        uint256 low = 0;

internal functions only called once can be inlined to save gas

  1. File: contracts/HolyPaladinToken.sol (line 714) 
    function _updateDropPerSecond() internal returns (uint256){
  2. File: contracts/HolyPaladinToken.sol (line 961) 
    function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256){
  3. File: contracts/HolyPaladinToken.sol (line 1077) 
    function _unstake(address user, uint256 amount, address receiver) internal returns(uint256) {
  4. File: contracts/HolyPaladinToken.sol (line 1235) 
    function _unlock(address user) internal {
  5. File: contracts/HolyPaladinToken.sol (line 1270) 
    function _kick(address user, address kicker) internal {

internal functions not called by the contract should be removed to save deployment gas

  1. File: contracts/HolyPaladinToken.sol (line 993) 
    function _getPastDelegate(address account, uint256 blockNumber) internal view returns (address) {

State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second access of a state variable within a function. Less obvious optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, or having local caches of state variable contracts/addresses.

  1. File: contracts/HolyPaladinToken.sol (line 272) 
        uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;
  2. File: contracts/HolyPaladinToken.sol (line 288) 
        uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;
  3. File: contracts/HolyPaladinToken.sol (line 350) 
        uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;
  4. File: contracts/HolyPaladinToken.sol (line 456) 
        uint256 lastUserLockIndex = userLocks[user].length - 1;
  5. File: contracts/HolyPaladinToken.sol (line 568) 
        uint256 lastUserLockIndex = userLocks[user].length - 1;
  6. File: contracts/HolyPaladinToken.sol (line 632) 
        uint256 lockAmount = userLocks[user][nbLocks - 1].amount;
  7. File: contracts/HolyPaladinToken.sol (line 709) 
        uint256 lastUserLockIndex = userLocks[user].length - 1;
  8. File: contracts/HolyPaladinToken.sol (line 813) 
                    vars.lastUserLockIndex = userLocks[user].length - 1;
  9. File: contracts/HolyPaladinToken.sol (line 935) 
        if (userLocks[account][nbCheckpoints - 1].fromBlock <= blockNumber) {
  10. File: contracts/HolyPaladinToken.sol (line 1148) 
            userLocks[user].push(UserLock(
  11. File: contracts/HolyPaladinToken.sol (line 1241) 
        uint256 currentUserLockIndex = userLocks[user].length - 1;
  12. File: contracts/HolyPaladinToken.sol (line 1276) 
        uint256 currentUserLockIndex = userLocks[user].length - 1;
  13. File: contracts/HolyPaladinToken.sol (line 1347) 
            uint256 currentUserLockIndex = userLocks[msg.sender].length - 1;
  14. File: contracts/HolyPaladinToken.sol (line 1165) 
                safe224(currentTotalLocked),
  15. File: contracts/HolyPaladinToken.sol (line 1251) 
            safe224(currentTotalLocked),
  16. File: contracts/HolyPaladinToken.sol (line 1288) 
            safe224(currentTotalLocked),
  17. File: contracts/HolyPaladinToken.sol (line 1353) 
                safe224(currentTotalLocked),
  18. File: contracts/HolyPaladinToken.sol (line 484) 
        return totalLocks[totalLocks.length - 1];
  19. File: contracts/HolyPaladinToken.sol (line 506) 
        if (totalLocks[nbCheckpoints - 1].fromBlock <= blockNumber) {
  20. File: contracts/HolyPaladinToken.sol (line 1225) 
                totalLocks.push(TotalLock(
  21. File: contracts/HolyPaladinToken.sol (line 914) 
        _moveDelegates(delegates[from], delegates[to], amount);
  22. File: contracts/HolyPaladinToken.sol (line 624) 
        uint256 currentVotes = nbCheckpoints == 0 ? 0 : checkpoints[user][nbCheckpoints - 1].votes;
  23. File: contracts/HolyPaladinToken.sol (line 969) 
        if (checkpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) {
  24. File: contracts/HolyPaladinToken.sol (line 1030) 
                uint256 oldVotes = nbCheckpoints == 0 ? 0 : checkpoints[from][nbCheckpoints - 1].votes;
  25. File: contracts/HolyPaladinToken.sol (line 1051) 
        if (pos > 0 && checkpoints[delegatee][pos - 1].fromBlock == block.number) {
  26. File: contracts/HolyPaladinToken.sol (line 678) 
        if (delegateCheckpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) {
  27. File: contracts/HolyPaladinToken.sol (line 1001) 
        if (delegateCheckpoints[account][nbCheckpoints - 1].fromBlock <= blockNumber) {
  28. File: contracts/HolyPaladinToken.sol (line 720) 
                currentDropPerSecond = endDropPerSecond;
  29. File: contracts/HolyPaladinToken.sol (line 727) 
        if(block.timestamp < lastDropUpdate + MONTH) return currentDropPerSecond; // Update it once a month
  30. File: contracts/HolyPaladinToken.sol (line 727) 
        if(block.timestamp < lastDropUpdate + MONTH) return currentDropPerSecond; // Update it once a month
  31. File: contracts/HolyPaladinToken.sol (line 729) 
        uint256 dropDecreasePerMonth = (startDropPerSecond - endDropPerSecond) / (dropDecreaseDuration / MONTH);
  32. File: contracts/HolyPaladinToken.sol (line 388) 
        uint256 claimAmount = amount < claimableRewards[msg.sender] ? amount : claimableRewards[msg.sender];
  33. File: contracts/HolyPaladinToken.sol (line 589) 
        uint256 estimatedClaimableRewards = claimableRewards[user];
  34. File: contracts/HolyPaladinToken.sol (line 1216) 
                userBonusRatioDecrease[user] = (userLockBonusRatio - baseLockBonusRatio) / duration;
  35. File: contracts/HolyPaladinToken.sol (line 1157) 
            uint256 userLockBonusRatio = minLockBonusRatio + (((maxLockBonusRatio - minLockBonusRatio) * durationRatio) / UNIT);
  36. File: contracts/HolyPaladinToken.sol (line 1213) 
                uint256 userLockBonusRatio = minLockBonusRatio + (((maxLockBonusRatio - minLockBonusRatio) * durationRatio) / UNIT);

Splitting require() statements that use && saves gas

See this issue for an example

  1. File: contracts/HolyPaladinToken.sol (line 1271) 
        require(user != address(0) && kicker != address(0), "hPAL: Address Zero");

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

  1. File: contracts/HolyPaladinToken.sol (line 47) 
        uint128 amount; // safe because PAL max supply is 10M tokens
  2. File: contracts/HolyPaladinToken.sol (line 49) 
        uint48 startTimestamp;
  3. File: contracts/HolyPaladinToken.sol (line 51) 
        uint48 duration;
  4. File: contracts/HolyPaladinToken.sol (line 53) 
        uint32 fromBlock; // because we want to search by block number
  5. File: contracts/HolyPaladinToken.sol (line 62) 
        uint224 total;
  6. File: contracts/HolyPaladinToken.sol (line 64) 
        uint32 fromBlock;
  7. File: contracts/HolyPaladinToken.sol (line 77) 
        uint32 fromBlock;
  8. File: contracts/HolyPaladinToken.sol (line 78) 
        uint224 votes;
  9. File: contracts/HolyPaladinToken.sol (line 83) 
        uint32 fromBlock;
  10. File: contracts/HolyPaladinToken.sol (line 1054) 
            uint32 blockNumber = safe32(block.number);
  11. File: contracts/HolyPaladinToken.sol (line 1387) 
    function safe32(uint n) internal pure returns (uint32) {
  12. File: contracts/HolyPaladinToken.sol (line 1392) 
    function safe48(uint n) internal pure returns (uint48) {
  13. File: contracts/HolyPaladinToken.sol (line 1397) 
    function safe128(uint n) internal pure returns (uint128) {
  14. File: contracts/HolyPaladinToken.sol (line 1402) 
    function safe224(uint n) internal pure returns (uint224) {

Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code

  1. File: contracts/HolyPaladinToken.sol (line 17) 
    uint256 public constant WEEK = 604800;
  2. File: contracts/HolyPaladinToken.sol (line 19) 
    uint256 public constant MONTH = 2629800;
  3. File: contracts/HolyPaladinToken.sol (line 21) 
    uint256 public constant UNIT = 1e18;
  4. File: contracts/HolyPaladinToken.sol (line 23) 
    uint256 public constant MAX_BPS = 10000;
  5. File: contracts/HolyPaladinToken.sol (line 25) 
    uint256 public constant ONE_YEAR = 31557600;
  6. File: contracts/HolyPaladinToken.sol (line 28) 
    uint256 public constant COOLDOWN_PERIOD = 864000; // 10 days
  7. File: contracts/HolyPaladinToken.sol (line 31) 
    uint256 public constant UNSTAKE_PERIOD = 432000; // 5 days
  8. File: contracts/HolyPaladinToken.sol (line 34) 
    uint256 public constant UNLOCK_DELAY = 1209600; // 2 weeks
  9. File: contracts/HolyPaladinToken.sol (line 37) 
    uint256 public constant MIN_LOCK_DURATION = 7889400; // 3 months
  10. File: contracts/HolyPaladinToken.sol (line 39) 
    uint256 public constant MAX_LOCK_DURATION = 63115200; // 2 years
  11. File: contracts/HolyPaladinToken.sol (line 114) 
    uint256 public immutable startDropPerSecond;
  12. File: contracts/HolyPaladinToken.sol (line 122) 
    uint256 public immutable dropDecreaseDuration;
  13. File: contracts/HolyPaladinToken.sol (line 124) 
    uint256 public immutable startDropTimestamp;
  14. File: contracts/HolyPaladinToken.sol (line 134) 
    uint256 public immutable baseLockBonusRatio;
  15. File: contracts/HolyPaladinToken.sol (line 136) 
    uint256 public immutable minLockBonusRatio;
  16. File: contracts/HolyPaladinToken.sol (line 138) 
    uint256 public immutable maxLockBonusRatio;

Duplicated require()/revert() checks should be refactored to a modifier or function

  1. File: contracts/HolyPaladinToken.sol (line 286) 
        require(userLocks[msg.sender].length != 0, "hPAL: No Lock");
  2. File: contracts/HolyPaladinToken.sol (lines 668-671) 
        require(
        blockNumber < block.number,
        "hPAL: invalid blockNumber"
    );
  3. File: contracts/HolyPaladinToken.sol (line 1078) 
        require(amount > 0, "hPAL: Null amount");
  4. File: contracts/HolyPaladinToken.sol (line 1343) 
        require(receiver != address(0), "hPAL: Address Zero");
  5. File: contracts/HolyPaladinToken.sol (line 1236) 
        require(user != address(0)); //Never supposed to happen, but security check
  6. File: contracts/HolyPaladinToken.sol (line 1272) 
        require(userLocks[user].length > 0, "hPAL: No Lock");
  7. File: contracts/HolyPaladinToken.sol (line 1280) 
        require(block.timestamp > userCurrentLockEnd, "hPAL: Not expired");
  8. File: contracts/HolyPaladinToken.sol (line 1281) 
        require(currentUserLock.amount > 0, "hPAL: No Lock");
  9. File: contracts/PaladinRewardReserve.sol (line 45) 
        require(approvedSpenders[spender], "Not approved Spender");

Multiplication/division by two should use bit shifting

<x> * 2 is equivalent to <x> << 1 and <x> / 2 is the same as <x> >> 1

  1. File: contracts/HolyPaladinToken.sol (line 841) 
                            vars.periodBonusRatio = newBonusRatio + ((vars.userRatioDecrease + vars.bonusRatioDecrease) / 2);

require() or revert() statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables

  1. File: contracts/HolyPaladinToken.sol (line 313) 
        require(msg.sender != user, "hPAL: cannot kick yourself");
  2. File: contracts/HolyPaladinToken.sol (line 385) 
        require(amount > 0, "hPAL: incorrect amount");
  3. File: contracts/HolyPaladinToken.sol (line 1142) 
        require(duration >= MIN_LOCK_DURATION, "hPAL: Lock duration under min");
  4. File: contracts/HolyPaladinToken.sol (line 1143) 
        require(duration <= MAX_LOCK_DURATION, "hPAL: Lock duration over max");
  5. File: contracts/HolyPaladinToken.sol (line 1342) 
        require(amount > 0, "hPAL: Null amount");
  6. File: contracts/HolyPaladinToken.sol (line 1343) 
        require(receiver != address(0), "hPAL: Address Zero");

Use custom errors rather than revert()/require() strings to save deployment gas

  1. File: contracts/HolyPaladinToken.sol (Various lines throughout the file)
  2. File: contracts/PaladinRewardReserve.sol (Various lines throughout the file)

Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

  1. File: contracts/HolyPaladinToken.sol (line 1416) 
    function setKickRatio(uint256 newKickRatioPerWeek) external onlyOwner {
  2. File: contracts/HolyPaladinToken.sol (line 1425) 
    function triggerEmergencyWithdraw(bool trigger) external onlyOwner {
  3. File: contracts/HolyPaladinToken.sol (line 1433) 
    function setEndDropPerSecond(uint256 newEndDropPerSecond) external onlyOwner {
  4. File: contracts/PaladinRewardReserve.sol (line 28) 
    function setNewSpender(address token, address spender, uint256 amount) external onlyOwner {
  5. File: contracts/PaladinRewardReserve.sol (line 36) 
    function updateSpenderAllowance(address token, address spender, uint256 amount) external onlyOwner {
  6. File: contracts/PaladinRewardReserve.sol (line 44) 
    function removeSpender(address token, address spender) external onlyOwner {
  7. File: contracts/PaladinRewardReserve.sol (line 52) 
    function transferToken(address token, address receiver, uint256 amount) external onlyOwner nonReentrant {

Not using the named return variables when a function returns, wastes deployment gas

  1. File: contracts/HolyPaladinToken.sol (lines 557-561) 
            return(
            balanceOf(user),
            0,
            balanceOf(user)
        );
  2. File: contracts/HolyPaladinToken.sol (lines 569-573) 
        return(
        balanceOf(user),
        uint256(userLocks[user][lastUserLockIndex].amount),
        balanceOf(user) - uint256(userLocks[user][lastUserLockIndex].amount)
    );
  3. File: contracts/HolyPaladinToken.sol (lines 557-561) 
            return(
            balanceOf(user),
            0,
            balanceOf(user)
        );
  4. File: contracts/HolyPaladinToken.sol (lines 569-573) 
        return(
        balanceOf(user),
        uint256(userLocks[user][lastUserLockIndex].amount),
        balanceOf(user) - uint256(userLocks[user][lastUserLockIndex].amount)
    );
  5. File: contracts/HolyPaladinToken.sol (lines 557-561) 
            return(
            balanceOf(user),
            0,
            balanceOf(user)
        );
  6. File: contracts/HolyPaladinToken.sol (lines 569-573) 
        return(
        balanceOf(user),
        uint256(userLocks[user][lastUserLockIndex].amount),
        balanceOf(user) - uint256(userLocks[user][lastUserLockIndex].amount)
    );
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)