Paladin - Warden Pledges contest - 0x1f8b's results

Judge has assessed an item in Issue #20 as M risk. The relevant finding follows:

2. Ownable and Pausable The contract WardenPledge is Ownable and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while the contract is paused should be avoided.

Affected source code:

WardenPledge.sol:18

• Low
  • 1. Outdated compiler
  • 2. Ownable and Pausable
  • 3. Lack of checks supportsInterface
  • 4. Lack of checks address0
  • 5. Lack of checks integer ranges
  • 6. Owners can steal the rewards
• Non critical
  • 7. Use solidity literals instead of math
  • 8. Avoid integer underflow
  • 9. Improve coding style

Low

1. Outdated compiler

The pragma version used is:

pragma solidity 0.8.10;

The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

0.8.17

• Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.

Apart from these, there are several minor bug fixes and improvements.

2. Ownable and Pausable

The contract WardenPledge is Ownable and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while the contract is paused should be avoided.

Affected source code:

• WardenPledge.sol:18

3. Lack of checks supportsInterface

The EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.

Reference:

• https://eips.ethereum.org/EIPS/eip-165

Affected source code:

• WardenPledge.sol:137-138

4. Lack of checks address(0)

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• WardenPledge.sol:137-138
• WardenPledge.sol:140

5. Lack of checks integer ranges

The following methods lack checks on the following integer arguments, you can see the recommendations above.

Affected source code:

minTargetVotes is allowed to be 0 during the constructor, but when this will be changed using updateMinTargetVotes will not be allowed.

• WardenPledge.sol:142

6. Owners can steal the rewards

The WardenPledge contract has a recoverERC20 method through which the owner can take an arbitrary erc20 token, this method contains a protection to prevent it from being a reward token, however, if the owner calls removeRewardToken first, owner will have no problem in being able to subtract the balance.

Affected source code:

• WardenPledge.sol:654

---

Non critical

7. Use solidity literals instead of math

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

Reference:

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

Affected source code:

• WardenPledge.sol:24

8. Avoid integer underflow

In the method _pledge, when endTimestamp is less than block.timestamp it will fault with a non custom error.

Affected source code:

• WardenPledge.sol:237

9. Improve coding style

Move all structures to a different file or to the top of the contract to improve code readability.

Affected source code:

• WardenPledge.sol:279-285

• Gas
  • 1. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 4 = 52
  • 2. Avoid unused returns
  • 3. delete optimization
    • Total gas saved: 5 * 3 = 15
  • 4. Use constant for static value
  • 5. Optimize _pledge with unchecked
  • 6. Optimize createPledge
  • 7. Optimize extendPledge with unchecked
  • 8. Use msg.sender instead of the cached creator
  • 9. Optimize increasePledgeRewardPerVote with unchecked
  • 10. Optimize updateRewardToken
  • 11. Avoid creating a new variable to emit an event

Gas

1. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
uint private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• WardenPledge.sol:268
• WardenPledge.sol:340
• WardenPledge.sol:401
• WardenPledge.sol:445

Total gas saved: 13 * 4 = 52

2. Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• WardenPledge.sol:653-661

3. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

Affected source code:

• WardenPledge.sol:473
• WardenPledge.sol:506
• WardenPledge.sol:589

Total gas saved: 5 * 3 = 15

4. Use constant for static value

The following variables are state variables, their value doesn't change but they weren't defined as constants, changing the definition to constant will save a lot of gas, because it won't use storage.

Affected source code:

• WardenPledge.sol:79

5. Optimize _pledge with unchecked

By applying the following changes it is possible to save gas:

        ...
        // Re-calculate the new Boost bias & slope (using Boostv2 logic)
        uint256 slope = amount / boostDuration;
+       uint256 bias;
+       unchecked { // It was previously checked
-       uint256 bias = slope * boostDuration;
+           bias = slope * boostDuration;
+       }

        // Rewards are set in the Pledge as reward/veToken/sec
        // To find the total amount of veToken delegated through the whole Boost duration
        // based on the Boost bias & the Boost duration, to take in account that the delegated amount decreases
        // each second of the Boost duration
        uint256 totalDelegatedAmount = ((bias * boostDuration) + bias) / 2;
        // Then we can calculate the total amount of rewards for this Boost
        uint256 rewardAmount = (totalDelegatedAmount * pledgeParams.rewardPerVote) / UNIT;

        if(rewardAmount > pledgeAvailableRewardAmounts[pledgeId]) revert Errors.RewardsBalanceTooLow();
+       unchecked { // It was previously checked
        pledgeAvailableRewardAmounts[pledgeId] -= rewardAmount;
+       }

        // Send the rewards to the user
        IERC20(pledgeParams.rewardToken).safeTransfer(user, rewardAmount);

        emit Pledged(pledgeId, user, amount, endTimestamp);
    }

Affected source code:

• WardenPledge.sol:268

6. Optimize createPledge

By applying the following changes it is possible to save gas by avoiding access to storage.

    function createPledge(
        address receiver,
        address rewardToken,
        uint256 targetVotes,
        uint256 rewardPerVote, // reward/veToken/second
        uint256 endTimestamp,
        uint256 maxTotalRewardAmount,
        uint256 maxFeeAmount
    ) external whenNotPaused nonReentrant returns(uint256){
        ...

        if(receiver == address(0) || rewardToken == address(0)) revert Errors.ZeroAddress();
        if(targetVotes < minTargetVotes) revert Errors.TargetVoteUnderMin();
-       if(minAmountRewardToken[rewardToken] == 0) revert Errors.TokenNotWhitelisted();
-       if(rewardPerVote < minAmountRewardToken[rewardToken]) revert Errors.RewardPerVoteTooLow();
+       if(rewardPerVote == 0 || rewardPerVote <= minAmountRewardToken[rewardToken]) revert Errors.RewardPerVoteTooLow();
        ...
    }

Affected source code:

• WardenPledge.sol:312-313

7. Optimize extendPledge with unchecked

By applying the following changes it is possible to save gas:

    function extendPledge(
        uint256 pledgeId,
        uint256 newEndTimestamp,
        uint256 maxTotalRewardAmount,
        uint256 maxFeeAmount
    ) external whenNotPaused nonReentrant {
        ...
        uint256 oldEndTimestamp = pledgeParams.endTimestamp;
        if(newEndTimestamp != _getRoundedTimestamp(newEndTimestamp) || newEndTimestamp < oldEndTimestamp) revert Errors.InvalidEndTimestamp();

+       uint256 addedDuration;
+       unchecked { // It was previously checked
-       uint256 addedDuration = newEndTimestamp - oldEndTimestamp;
-           addedDuration = newEndTimestamp - oldEndTimestamp;
+       }
        ...
    }

Affected source code:

• WardenPledge.sol:385

8. Use msg.sender instead of the cached creator

Using msg.sender will always be cheaper than using a local variable.

The reason is the following:

• The CALLER operation which is reading the msg.sender global variable costs 2.
• While MLOAD/MSTORE operations which are memory operations (storage where local variables are stored) costs 3.

Affected source code:

• WardenPledge.sol:333
• WardenPledge.sol:335
• WardenPledge.sol:352
• WardenPledge.sol:353
• WardenPledge.sol:355
• WardenPledge.sol:394
• WardenPledge.sol:396
• WardenPledge.sol:438
• WardenPledge.sol:440

9. Optimize increasePledgeRewardPerVote with unchecked

By applying the following changes it is possible to save gas:

    function increasePledgeRewardPerVote(
        uint256 pledgeId,
        uint256 newRewardPerVote,
        uint256 maxTotalRewardAmount,
        uint256 maxFeeAmount
    ) external whenNotPaused nonReentrant {
        ...
        uint256 oldRewardPerVote = pledgeParams.rewardPerVote;
        if(newRewardPerVote <= oldRewardPerVote) revert Errors.RewardsPerVotesTooLow();
        uint256 remainingDuration = pledgeParams.endTimestamp - block.timestamp;
+       uint256 rewardPerVoteDiff;
+       unchecked { // It was previously checked
-       uint256 rewardPerVoteDiff = newRewardPerVote - oldRewardPerVote;
+           rewardPerVoteDiff = newRewardPerVote - oldRewardPerVote;
+       }
        ...
    }

Affected source code:

• WardenPledge.sol:431

10. Optimize updateRewardToken

It is possible to optimize the input verification order in the following way:

    function updateRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {
        if(token == address(0)) revert Errors.ZeroAddress();
+       if(minRewardPerSecond == 0) revert Errors.InvalidValue();
        if(minAmountRewardToken[token] == 0) revert Errors.NotAllowedToken();
-       if(minRewardPerSecond == 0) revert Errors.InvalidValue();

        minAmountRewardToken[token] = minRewardPerSecond;

        emit UpdateRewardToken(token, minRewardPerSecond);
    }

Affected source code:

• WardenPledge.sol:573

11. Avoid creating a new variable to emit an event

It's possible to emit the event before the change and save one variable creation.

    function updateChest(address chest) external onlyOwner {
        if(chest == address(0)) revert Errors.ZeroAddress();
-       address oldChest = chestAddress;
+       emit ChestUpdated(chestAddress, chest);
        chestAddress = chest;
-       emit ChestUpdated(oldChest, chest);
    }

    function updateMinTargetVotes(uint256 newMinTargetVotes) external onlyOwner {
        if(newMinTargetVotes == 0) revert Errors.InvalidValue();
-       uint256 oldMinTarget = minTargetVotes;
+       emit MinTargetUpdated(minTargetVotes, newMinTargetVotes);
        minTargetVotes = newMinTargetVotes;
-       emit MinTargetUpdated(oldMinTarget, newMinTargetVotes);
    }

    function updatePlatformFee(uint256 newFee) external onlyOwner {
        if(newFee > 500) revert Errors.InvalidValue();
-       uint256 oldfee = protocalFeeRatio;
+       emit PlatformFeeUpdated(protocalFeeRatio, newFee);
        protocalFeeRatio = newFee;
-       emit PlatformFeeUpdated(oldfee, newFee);
    }

Affected source code:

• WardenPledge.sol:604
• WardenPledge.sol:617
• WardenPledge.sol:630