Paladin - Warden Pledges contest - RaymondFam's results

Time Units

According to:

https://docs.soliditylang.org/en/v0.8.14/units-and-global-variables.html

suffixes like seconds, minutes, hours, days and weeks after literal numbers can be used to specify units of time where seconds are the base unit and units are considered naively in the following way:

1 == 1 seconds 1 minutes == 60 seconds 1 hours == 60 minutes 1 days == 24 hours 1 weeks == 7 days

To avoid human error while making the assignment more verbose, the following constant declarations may respectively be rewritten as:

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

    uint256 public constant WEEK = 1 weeks;

block.timestamp Unreliable

The use of block.timestamp as part of the time checks can be slightly altered by miners/validators to favor them in contracts that have logic strongly dependent on them.

Consider taking into account this issue and warning the users that such a scenario could happen. If the alteration of timestamps cannot affect the protocol in any way, consider documenting the reasoning and writing tests enforcing that these guarantees will be preserved even if the code changes in the future.

There are a total of eight instances entailed:

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L229 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L237 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L319 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L380 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L426 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L430 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L463 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L496

Sanity Checks Upon Deployment

Zero address and zero value checks should be implemented at the constructor to avoid human error(s) that could result in non-functional calls associated with them. The constructor should be refactored to:

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

    constructor(
        address _votingEscrow,
        address _delegationBoost,
        address _chestAddress,
        uint256 _minTargetVotes
    ) {
        if (_votingEscrow == address(0)|| _delegationBoost == address(0) || _chestAddress == address(0)) revert Errors.ZeroAddress();
        if(_minTargetVotes == 0) revert Errors.InvalidValue();
        
        votingEscrow = IVotingEscrow(_votingEscrow);
        delegationBoost = IBoostV2(_delegationBoost);

        chestAddress = _chestAddress;

        minTargetVotes = _minTargetVotes;
    }

Doing this will be consistent with the sanity checks implemented in the setter functions, updateChest() and updateMinTargetVotes().

Better yet, incorporate complementary codehash checks too just to make sure the address inputs are the matching ones if these have not been deemed an overkill from the developer team's perspective.

Add a Timelock to Critical Parameter Change

It is a good practice to give time for users to react and adjust to critical changes with a mandatory time window between them. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period. This allows users that do not accept the change to withdraw within the grace period. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious Owner making any malicious or ulterior intention). Specifically, privileged roles could use front running to make malicious changes just ahead of incoming transactions, or purely accidental negative effects could occur due to the unfortunate timing of changes.

Here are the instances entailed:

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

Contract Owner Has Too Many Privileges

The owner of the contracts has too many privileges relative to standard users. The consequence is disastrous if the contract owner's private key has been compromised. And, in the event the key was lost or unrecoverable, no implementation upgrades and system parameter updates will ever be possible.

For a project this grand, it increases the likelihood that the owner will be targeted by an attacker, especially given the insufficient protection on sensitive owner private keys. The concentration of privileges creates a single point of failure; and, here are some of the incidents that could possibly transpire:

Transfer ownership and mess up with all the setter functions, hijacking the entire protocol.

Consider:

1. splitting privileges (e.g. via the multisig option) to ensure that no one address has excessive ownership of the system,
2. clearly documenting the functions and implementations the owner can change,
3. documenting the risks associated with privileged users and single points of failure, and
4. ensuring that users are aware of all the risks associated with the system.

Typo Errors

            @ balacne
Line 292    * @param targetVotes Maximum taget of votes to have (own balacne + delegation) for the receiver
            @ ot
Line 365    * @param maxTotalRewardAmount Maximum added total reward amount allowed ot be pulled by this contract
            @ ot
Line 366    * @param maxFeeAmount Maximum fee amount allowed ot be pulled by this contract

Function Calls in Loop Could Lead to Denial of Service

Function calls made in unbounded loop are error-prone with potential resource exhaustion as it can trap the contract due to gas limitations or failed transactions. Here is the one instance entailed:

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

Consider bounding the loop where possible to avoid unnecessary gas wastage and denial of service.

Non-Changing State Variable Should be Declared as Constant

minDelegationTime should be declared constant and capitalized considering it is not meant to be update in the contract codebase.

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

It has only be used in the following two conditional statements:

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

Use delete to Clear Variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic. Consider replacing assignments of zero with delete statements. Here are some the three instances entailed:

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

Un-indexed Parameters in Events

Consider indexing parameters for events, serving as logs filter when looking for specifically wanted data. Up to three parameters in an event function can receive the attribute indexed which will cause the respective arguments to be treated as log topics instead of data. There are two instances entailed:

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L85-L92 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L115-L119

Non-descriptive Comments

Comments should be brief but they should adequately portray the intended purposes where possible. All comments associated with events in the contract are found to be incomplete ending with ... xx */.

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

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =). There are a total of five instances associated with >= entailed:

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L223 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L374 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L420 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L457 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L489

As an example, consider replacing >= with the strict counterpart > in line 489:

        if(pledgeId > pledgesIndex() - 1) revert Errors.InvalidPledgeID();

There are also a total of five instances associated with <= entailed:

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L229 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L380 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L426 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L429 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L496

As an example, consider replacing <= with the strict counterpart < in line 496:

        if(pledgeParams.endTimestamp < block.timestamp + 1) revert Errors.ExpiredPledge();

+= and -= Costs More Gas

+= generally costs 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop (which is fortunately not found in the contract in scope).

There are also a total of three instances associated with += entailed:

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

As an example, the following line of code in line 401 could be rewritten as:

        pledgeAvailableRewardAmounts[pledgeId] = pledgeAvailableRewardAmounts[pledgeId] + totalRewardAmount;

There is one instance associated with -= entailed which could be rewritten as:

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

        pledgeAvailableRewardAmounts[pledgeId] = pledgeAvailableRewardAmounts[pledgeId] - rewardAmount;

Function Order Affects Gas Consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

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

Activate the Optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
  solidity: {
    version: "0.8.11",
    settings: {
      optimizer: {
        enabled: true,
        runs: 1000,
      },
    },
  },
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization PUSH1 0x00 DUP2 EQ ISZERO PUSH1 [cont offset] JUMPI

after optimization DUP1 PUSH1 [revert offset] JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, 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. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Use Storage Instead of Memory for Structs/Arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

There are a total of two instances entailed:

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

Unchecked SafeMath Saves Gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ... } to use the previous wrapping behavior further saves gas.

Here are some of the instances entailed:

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

As an example, line 445 could be rewritten as:

    unchecked { pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount; }

Use of Named Returns for Local Variables Saves Gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable. There are a total of seven instances entailed:

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L153 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L163 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L172 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L181 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L307 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L653 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L665

As an example, lines 307 and 337 of createPledge() can be refactored via the chain assignment method:

Line 307    ) external whenNotPaused nonReentrant returns(uint256 newPledgeId){

Line 337    newPledgeId = vars.newPledgeID = pledgesIndex();

Note: Line 357 may therefore be removed.

Payable Access Control Functions Costs Less Gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

There are a total of ten instances entailed:

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L541 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L560 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L570 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L585 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L599 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L612 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L625 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L636 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L643 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L653

State Variables Repeatedly Read Should be Cached

SLOADs are cost 100 gas each after the 1st one whereas MLOADs/MSTOREs only incur 3 gas each. As such, storage values read multiple times should be cached in the stack memory the first time (costing only 1 SLOAD) and then re-read from this cache to avoid multiple SLOADs.

delegationBoost should be cached as follows and inserted before line 240:

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

        IBoostV2 delegationBoost = delegationBoost;

Similarly, pledgeAvailableRewardAmounts[pledgeId] should be cached and have the following code block refactored to:

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

        uint256 remainingAmount = pledgeAvailableRewardAmounts[pledgeId];

        if(rewardAmount > remainingAmount) revert Errors.RewardsBalanceTooLow();
        remainingAmount -= rewardAmount;

Strangely, pledgeAvailableRewardAmounts[pledgeId] has been cached in the following two code blocks but was not fully utilized:

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L466-L473 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L502-L506

The cached variable should logically be used in lines 473 and 506.

minAmountRewardToken should likewise be cached in the following four instances:

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L312-L313 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L526-L530 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L572-L575 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L587-L589

`protocalFeeRatio' should also be cached as follows and inserted before line 625:

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

        unit256 protocalFeeRatio = protocalFeeRatio;

chestAddress should likewise be cached in the following code block:

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

minTargetVotes should likewise be cached in the following code block:

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

Unused Event

IncreasePledgeTargetVotes() has not been found emitting in WardenPledge.sol. Consider emitting it where appropriate or having it removed from the contract.

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