Paladin - Warden Pledges contest - 0xSmartContract's results

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L18

Vulnerability details

Impact

The pause() function on WardenPledge.sol has a single point of failure and onlyOwner can stop all project.

Owner is not behind a multisig and changes are not behind a timelock.(This information hasnt got in documents)

Even if protocol admins/developers are not malicious there is still a chance for admin keys to be stolen.

   function pause() external onlyOwner {
        _pause();
    }

In addition, when we add other onlyOwner privileges, the single point of failure situation becomes stronger;

onlyOwner   |   Functions
========================
onlyOwner  =>  addMultipleRewardToken(address[],uint256[])
onlyOwner  =>  addRewardToken(address,uint256)
onlyOwner  =>  updateRewardToken(address,uint256)
onlyOwner  =>  removeRewardToken(address)
onlyOwner  =>  updateChest(address)
onlyOwner  =>  updateMinTargetVotes(uint256)
onlyOwner  =>  updatePlatformFee(uint256)
onlyOwner  =>  pause()
onlyOwner  =>  unpause()
onlyOwner  =>  recoverERC20(address)

See this example where a similar finding has been flagged as a high-severity issue: realitycards-findings

Recommended Mitigation Steps

Isolate functionu and other lightweight onlyOwner functions such as pause() that are very powerful and will affect the project

Add a time lock to critical functions like pause()

Admin-only functions that change critical parameters should emit events and have timelocks. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.

Allow only multi-signature wallets to call the function to reduce the likelihood of an attack.

https://twitter.com/danielvf/status/1572963475101556738?s=20&t=V1kvzfJlsx-D2hfnG0OmuQ

Summary

Low Risk Issues List

Total 2 issues

Non-Critical Issues List

Total 9 issues

Suggestions

Total 2 suggestions

[L-01] There is no 0 check for very critical addresses in the constructor.

With the constructor in the WardenPledge.sol file, the initial addresses of chestAddress , minTargetVotes , delegationBoost and votingEscrow are set with an argument of type addresses, but there are no check that prevents this addresses from starting with 0. There is a risk that they are accidentally initialized to 0

Passing the addresses variables blank causes it to be set as 0 address.

The addresses initialized with 0 by mistake or forgetting cant be changed.

The biggest risk is; Until this value is found to be incorrect, the project can be used, users can trade, use the platform

contracts/WardenPledge.sol:
  130      */
  131:     constructor(
  132:         address _votingEscrow,
  133:         address _delegationBoost,
  134:         address _chestAddress,
  135:         uint256 _minTargetVotes
  136:     ) {
  137:         votingEscrow = IVotingEscrow(_votingEscrow);
  138:         delegationBoost = IBoostV2(_delegationBoost);
  139: 
  140:         chestAddress = _chestAddress;
  141: 
  142:         minTargetVotes = _minTargetVotes;
  143:     } 

Tools Used

Manual Code Review

Recommended Mitigation Steps

Add an if block to the constructor ;

error ZeroAddressError();

        if _votingEscrow = address(0) && _delegationBoost = address(0) && _chestAddress = address(0))
            ZeroAddressError();
}

[L-02] Very critical owner privileges can cause complete destruction of the project in a possible privateKey exploit

The contract’s owner is most impartant power role in this project. the owner is able to perform certain privileged activities.

However, owner privileges are numerous and there is no timelock structure in the process of using these privileges.

The owner is assumed to be an EOA, since the documents do not provide information on whether the owner will be a multisign structure.

In parallel with the private key thefts of the project owners, which have increased recently, this vulnerability has been stated as medium.

Similar vulnerability; Private keys stolen:

Hackers have stolen cryptocurrency worth around €552 million from a blockchain project linked to the popular online game Axie Infinity, in one of the largest cryptocurrency heists on record. Security issue : PrivateKey of the project officer was stolen: https://www.euronews.com/next/2022/03/30/blockchain-network-ronin-hit-by-552-million-crypto-heist

Proof of Concept

owner powers;

contracts/WardenPledge.sol:
  541:     function addMultipleRewardToken(address[] calldata tokens) external onlyOwner {
  560:     function addRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {
  570:     function updateRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {
  585:     function removeRewardToken(address token) external onlyOwner {
  599:     function updateChest(address chest) external onlyOwner {
  612:     function updateMinTargetVotes(uint256 newMinTargetVotes) external onlyOwner {
  625:     function updatePlatformFee(uint256 newFee) external onlyOwner {
  636:     function pause() external onlyOwner {
  643:     function unpause() external onlyOwner {
  653:     function recoverERC20(address token) external onlyOwner returns(bool) {

Recommendation;

1- A timelock contract should be added to use ``owner privileges. In this way, users can be warned in case of a possible security weakness.

2- owner can be a Multisign wallet and this part is specified in the documentation

[N-01] Function writing that does not comply with the Solidity Style Guide

Description: Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

Functions should be grouped according to their visibility and ordered:

constructor receive function (if exists) fallback function (if exists) external public internal private within a grouping, place the view and pure functions last

[N-02] Need Fuzzing test

Project uncheckeds list:

contracts/WardenPledge.sol:
 550:             unchecked{ ++i; }
  551          }

The functions used are critical. For this reason, there must be fuzzing tests in the tests of the project. Not seen in tests.

Recommendation: Use should fuzzing test like Echidna.

As Alberto Cuesta Canada said: Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now.

https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05

[N-03] Use a more recent version of Solidity

Description: For security, it is best practice to use the latest Solidity version. For the security fix list in the versions; https://github.com/ethereum/solidity/blob/develop/Changelog.md

Recommendation: Old version of Solidity is used (0.8.10), newer version can be used (0.8.17)

[N-04] Solidity compiler optimizations can be problematic

hardhat.config.ts:
  17  
  18: const config: HardhatUserConfig = {
  19:   defaultNetwork: "hardhat",
  20:   solidity: {
  21:     compilers: [
  22:       {
  23:         version: "0.8.10",
  24:         settings: {
  25:           optimizer: {
  26:             enabled: true,
  27:             runs: 200,
  28:           },
  29:         }
  30:       }

Description: Protocol has enabled optional compiler optimizations in Solidity. There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them.

Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten-generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG.

Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. More recently, another bug due to the incorrect caching of keccak256 was reported. A compiler audit of Solidity from November 2018 concluded that the optional optimizations may not be safe. It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations.

Exploit Scenario A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.

Recommendation: Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug. Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

[N-05] NatSpec is missing

NatSpec is missing for the _chestAddress

contracts/WardenPledge.sol:
  122  
  123:     // Constructor
  124: 
  125:     /**
  126:     * @dev Creates the contract, set the given base parameters
  127:     * @param _votingEscrow address of the voting token to delegate
  128:     * @param _delegationBoost address of the contract handling delegation
  129:     * @param _minTargetVotes min amount of veToken to target in a Pledge
  130:     */
  131:     constructor(
  132:         address _votingEscrow,
  133:         address _delegationBoost,
  134:         address _chestAddress,
  135:         uint256 _minTargetVotes

[N-06]  For functions, follow Solidity standard naming conventions

The above codes don't follow Solidity's standard naming convention, internal and private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

 function safe64(uint256 n) internal pure returns (uint64) {
        if(n > type(uint64).max) revert Errors.NumberExceed64Bits();
        return uint64(n);
    }

[N-07]  Missing Event for Critical Parameters Change

Events help non-contract tools to track changes, and events prevent users from being surprised by changes

contracts/WardenPledge.sol:
  635       */
  636:     function pause() external onlyOwner {
  637:         _pause();
  638:     }
  639: 
  640:     /**
  641:      * @notice Unpauses the contract
  642:      */
  643:     function unpause() external onlyOwner {
  644:         _unpause();
  645:     }

Recommendation: Add Event-Emit

[N-08]  Add NatSpec Mapping comment

Description: Add NatSpec comments describing mapping keys and values

Context:

4 results - 1 file

contracts/WardenPledge.sol:
  50:     mapping(uint256 => address) public pledgeOwner;
  52:     mapping(address => uint256[]) public ownerPledges;
  56:     mapping(uint256 => uint256) public pledgeAvailableRewardAmounts;
  67:     mapping(address => uint256) public minAmountRewardToken;

Recommendation:

 /// @dev    address(user) -> address(ERC0 Contract Address) -> uint256(allowance amount from user)
    mapping(address => mapping(address => uint256)) public allowance;

[N-09]   Same Contract imported twice

The SafeERC20 contract already inherits IERC20, it is also unnecessary to import IERC20.

contracts/WardenPledge.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity 0.8.10;
  3: 
  4: import "./oz/interfaces/IERC20.sol";
  5: import "./oz/libraries/SafeERC20.sol";

[S-01] Add to blacklist function

Description: Cryptocurrency mixing service, Tornado Cash, has been blacklisted in the OFAC. A lot of blockchain companies, token projects, NFT Projects have blacklisted all Ethereum addresses owned by Tornado Cash listed in the US Treasury Department's sanction against the protocol. https://home.treasury.owner/policy-issues/financial-sanctions/recent-actions/20220808

Some of these Projects; USDC Aave Uniswap Balancer Infura Alchemy Opensea dYdX

For this reason, every project in the Ethereum network must have a blacklist function, this is a good method to avoid legal problems in the future, apart from the current need.

Transactions from the project by an account funded by Tonadocash or banned by OFAC can lead to legal problems.Especially American citizens may want to get addresses to the blacklist legally, this is not an obligation

If you think that such legal prohibitions should be made directly by validators, this may not be possible: https://www.paradigm.xyz/2022/09/base-layer-neutrality

The ban on Tornado Cash makes little sense, because in the end, no one can prevent people from using other mixer smart contracts, or forking the existing ones. It neither hinders cybercrime, nor privacy.

Here is the most beautiful and close to the project example; Manifold

Manifold Contract https://etherscan.io/address/0xe4e4003afe3765aca8149a82fc064c0b125b9e5a#code

     modifier nonBlacklistRequired(address extension) {
         require(!_blacklistedExtensions.contains(extension), "Extension blacklisted");
         _;
     }

Recommended Mitigation Steps add to Blacklist function and modifier.

[S-02] Generate perfect code headers every time

Description: I recommend using header for Solidity code layout and readability

https://github.com/transmissions11/headers

/*//////////////////////////////////////////////////////////////
                           TESTING 123
//////////////////////////////////////////////////////////////*/

Summary

Gas Optimizations List

Total 9 issues

Suggestions

Total 1 suggestion

[G-01] No need return keyword for gas efficient

Context:

contracts/WardenPledge.sol:
  652      */
  653:     function recoverERC20(address token) external onlyOwner returns(bool) {
  654:         if(minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken();
  655: 
  656:         uint256 amount = IERC20(token).balanceOf(address(this));
  657:         if(amount == 0) revert Errors.NullValue();
  658:         IERC20(token).safeTransfer(owner(), amount);
  659: 
  660:         return true;
  661:     }

Recommendation: You should remove the return keyword from the specified context.

contracts/WardenPledge.sol:
  652      */
  653:     function recoverERC20(address token) external onlyOwner returns(bool) {
  654:         if(minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken();
  655: 
  656:         uint256 amount = IERC20(token).balanceOf(address(this));
  657:         if(amount == 0) revert Errors.NullValue();
  658:         IERC20(token).safeTransfer(owner(), amount);
  659: 
  660:         true;
  661:     }

[G-02] Functions guaranteed to revert_ when callled by normal users can be marked payable

Context: 

10 results - 1 file

contracts/WardenPledge.sol:
  540      */
  541:     function addMultipleRewardToken(address[] calldata tokens, uint256[] calldata minRewardsPerSecond) external onlyOwner {
  542          uint256 length = tokens.length;

  559      */
  560:     function addRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {
  561          _addRewardToken(token, minRewardPerSecond);

  569      */
  570:     function updateRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {
  571          if(token == address(0)) revert Errors.ZeroAddress();

  584      */
  585:     function removeRewardToken(address token) external onlyOwner {
  586          if(token == address(0)) revert Errors.ZeroAddress();

  598      */
  599:     function updateChest(address chest) external onlyOwner {
  600          if(chest == address(0)) revert Errors.ZeroAddress();

  611      */
  612:     function updateMinTargetVotes(uint256 newMinTargetVotes) external onlyOwner {
  613          if(newMinTargetVotes == 0) revert Errors.InvalidValue();

  624      */
  625:     function updatePlatformFee(uint256 newFee) external onlyOwner {
  626          if(newFee > 500) revert Errors.InvalidValue();

  635       */
  636:     function pause() external onlyOwner {
  637          _pause();

  642       */
  643:     function unpause() external onlyOwner {
  644          _unpause();

  652      */
  653:     function recoverERC20(address token) external onlyOwner returns(bool) {

Description: If a function modifier or require such as onlyOwner-admin 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. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

Recommendation: Functions guaranteed to revert when called by normal users can be marked payable  (for only onlyowner or admin functions)

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
        c0.foo();
        c1.foo();
    }
}

contract Contract0 {
    uint256inverse;
    
    function foo() external {
       inverse++;
    }
}

contract Contract1 {
    uint256inverse;
    
    function foo() external payable {
       inverse++;
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 44293                                     ┆ 252             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ foo                                       ┆ 22308           ┆ 22308 ┆ 22308  ┆ 22308 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 41893                                     ┆ 240             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ foo                                       ┆ 22284           ┆ 22284 ┆ 22284  ┆ 22284 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

[G-03] x -= y (x += y) costs more gas than x = x – y (x = x + y) for state variables

Description: x -= y costs more gas than x = x – y for state variables.

Context:

3 results - 1 file

contracts/WardenPledge.sol:
340:         pledgeAvailableRewardAmounts[vars.newPledgeID] += vars.totalRewardAmount;
401:         pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;
445:         pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTest is DSTest {
  
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
        c0.swap(2,3);
        c1.swap1(2,3);
    }
}
contract Contract0 {
    uint256 public amountIn = 10;
    
    
    function swap(uint256 amountInToBin, uint256 fee)  external returns (uint256 ) {
       
       return amountIn -= amountInToBin + fee;
    }
}

contract Contract1 {
    uint256 public amountIn = 10;
    
    function swap1(uint256 amountInToBin, uint256 fee)  external returns (uint256 result1) {
       return (amountIn = amountIn - (amountInToBin + fee));
    }
}

Gas Report:

╭──────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/test.sol:Contract0 contract ┆                 ┆      ┆        ┆      ┆         │
╞══════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                      ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 83017                                ┆ 341             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                        ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ swap                                 ┆ 5454            ┆ 5454 ┆ 5454   ┆ 5454 ┆ 1       │
╰──────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯
╭──────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/test.sol:Contract1 contract ┆                 ┆      ┆        ┆      ┆         │
╞══════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                      ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 83017                                ┆ 341             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                        ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ swap1                                ┆ 5431            ┆ 5431 ┆ 5431   ┆ 5431 ┆ 1       │
╰──────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯

[G-04] Optimize names to save gas [22 gas per instance]

Context:  All Contracts

Description:  Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions. 

Recommendation:  Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the WardenPledge.sol contract will be the most used; A lower method ID may be given.

Proof of Consept: https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

WardenPledge.sol function names can be named and sorted according to METHOD ID

Sighash   |   Function Signature
========================
2ce9f7bb  =>  pledgesIndex()
18ea71cf  =>  getUserPledges(address)
9482e6b2  =>  getAllPledges()
350fcecf  =>  _getRoundedTimestamp(uint256)
294806ad  =>  pledge(uint256,uint256,uint256)
e169aa07  =>  pledgePercent(uint256,uint256,uint256)
6c6c07ce  =>  _pledge(uint256,address,uint256,uint256)
8fb51ebc  =>  createPledge(address,address,uint256,uint256,uint256,uint256,uint256)
417dea25  =>  extendPledge(uint256,uint256,uint256,uint256)
bebedce9  =>  increasePledgeRewardPerVote(uint256,uint256,uint256,uint256)
48d03a6b  =>  retrievePledgeRewards(uint256,address)
05374a14  =>  closePledge(uint256,address)
54a0c47d  =>  _addRewardToken(address,uint256)
be1145c2  =>  addMultipleRewardToken(address[],uint256[])
0e3802e9  =>  addRewardToken(address,uint256)
ba82c897  =>  updateRewardToken(address,uint256)
3d509c97  =>  removeRewardToken(address)
eca5d0ca  =>  updateChest(address)
7ed463f0  =>  updateMinTargetVotes(uint256)
aa0b5988  =>  updatePlatformFee(uint256)
8456cb59  =>  pause()
3f4ba83a  =>  unpause()
9e8c708e  =>  recoverERC20(address)
a0042149  =>  safe64(uint256)

[G-05] Use assembly to check for address(0)

Context: 

7 results - 1 file

contracts/WardenPledge.sol:
  309  
  310:         if(receiver == address(0) || rewardToken == address(0)) revert Errors.ZeroAddress();
  311          if(targetVotes < minTargetVotes) revert Errors.TargetVoteUnderMin();

  459          if(msg.sender != creator) revert Errors.NotPledgeCreator();
  460:         if(receiver == address(0)) revert Errors.ZeroAddress();
  461  

  491          if(msg.sender != creator) revert Errors.NotPledgeCreator();
  492:         if(receiver == address(0)) revert Errors.ZeroAddress();
  493  

  526          if(minAmountRewardToken[token] != 0) revert Errors.AlreadyAllowedToken();
  527:         if(token == address(0)) revert Errors.ZeroAddress();
  528          if(minRewardPerSecond == 0) revert Errors.NullValue();

  570      function updateRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {
  571:         if(token == address(0)) revert Errors.ZeroAddress();
  572          if(minAmountRewardToken[token] == 0) revert Errors.NotAllowedToken();

  585      function removeRewardToken(address token) external onlyOwner {
  586:         if(token == address(0)) revert Errors.ZeroAddress();
  587          if(minAmountRewardToken[token] == 0) revert Errors.NotAllowedToken();

  599      function updateChest(address chest) external onlyOwner {
  600:         if(chest == address(0)) revert Errors.ZeroAddress();
  601          address oldChest = chestAddress;

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    function testGas() public view {
        c0.setOperator(address(this));
        c1.setOperator(address(this));
    }
}
contract Contract0 {
    function setOperator(address operator_) public pure {
        require(operator_) != address(0), "INVALID_RECIPIENT");    }
}
contract Contract1 {
    function setOperator(address operator_) public pure {
        assembly {
            if iszero(operator_) {
                mstore(0x00, "Callback_InvalidParams")
                revert(0x00, 0x20)
            }
        }
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 50899                                     ┆ 285             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setOperator                               ┆ 258             ┆ 258 ┆ 258    ┆ 258 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 44893                                     ┆ 255             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setOperator                               ┆ 252             ┆ 252 ┆ 252    ┆ 252 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯

[G-06] Use assembly to write address storage values

Context:

4 results - 1 file

contracts/WardenPledge.sol:
570:     function updateRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {
599:     function updateChest(address chest) external onlyOwner {
612:     function updateMinTargetVotes(uint256 newMinTargetVotes) external onlyOwner {
625:     function updatePlatformFee(uint256 newFee) external onlyOwner {

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTestFoundry is DSTest {
    Contract1 c1;
    Contract2 c2;
    
    function setUp() public {
        c1 = new Contract1();
        c2 = new Contract2();
    }
    
    function testGas() public {
        c1.setRewardTokenAndAmount(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4,356);
        c2.setRewardTokenAndAmount(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4,356);
    }
}

contract Contract1  {
    address rewardToken ;
    uint256 reward;

    function setRewardTokenAndAmount(address token_, uint256 reward_) external {
        rewardToken = token_;
        reward = reward_;
    }
}

contract Contract2  {
    address rewardToken ;
    uint256 reward;

    function setRewardTokenAndAmount(address token_, uint256 reward_) external {
        assembly {
            sstore(rewardToken.slot, token_)
            sstore(reward.slot, reward_)           

        }
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 50899                                     ┆ 285             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setRewardTokenAndAmount                   ┆ 44490           ┆ 44490 ┆ 44490  ┆ 44490 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract2 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 38087                                     ┆ 221             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setRewardTokenAndAmount                   ┆ 44457           ┆ 44457 ┆ 44457  ┆ 44457 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

[G-07] The solady Library's Ownable contract is significantly gas-optimized, which can be used

Description: The project uses the Ownable authorization model with the PendingOwnable.sol contract. I recommend using Solady's highly gas optimized contract.

https://github.com/Vectorized/solady/blob/main/src/auth/OwnableRoles.sol

[G-08] Setting The Constructor To Payable [13 gas per instance]

Description: You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 13 gas on deployment with no security risks.

Recommendation: Set the constructor to payable

Proof Of Concept: https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5?u=pcaversaccio

The optimizer was turned on and set to 10000 runs

contract GasTestFoundry is DSTest {
    
    Contract1 c1;
    Contract2 c2;
    
    function setUp() public {
        c1 = new Contract1();
        c2 = new Contract2();
    }
    
    function testGas() public {
        c1.x();
        c2.x();
    }
}

contract Contract1 {
    
    uint256 public dummy;
    constructor() payable {
        dummy = 1;
    }
    
    function x() public {

    }
}

contract Contract2 {
    
    uint256 public dummy;
    constructor() {
        dummy = 1;
    }
    
    function x() public {
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 49563                                     ┆ 159             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract2 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 49587                                     ┆ 172             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

[G-09] Stack variable used as a cheaper cache for a state variable is only used once

If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend There are 2 instances of this issue:

contracts/WardenPledge.sol:
  611      */
  612:     function updateMinTargetVotes(uint256 newMinTargetVotes) external onlyOwner {
  614:         uint256 oldMinTarget = minTargetVotes;
  615:         minTargetVotes = newMinTargetVotes;

  625:     function updatePlatformFee(uint256 newFee) external onlyOwner {
  627:         uint256 oldfee = protocalFeeRatio;
  628          protocalFeeRatio = newFee;

[S-01] Missing zero-address check in constructor

Description: Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly. It also wast gas as it requires the redeployment of the contract.

 constructor(
        address _votingEscrow,
        address _delegationBoost,
        address _chestAddress,
        uint256 _minTargetVotes
    ) {
        votingEscrow = IVotingEscrow(_votingEscrow);
        delegationBoost = IBoostV2(_delegationBoost);

        chestAddress = _chestAddress;

        minTargetVotes = _minTargetVotes;
    }