Paladin - Warden Pledges contest - ajtra's results

Summary

Low

1. L01 - Missing checks for address(0x0) when assigning values to address state variables

Non Critical

2. NC01 - Event is missing indexed fields
3. NC02 - Outdated compiler version
4. NC03 - No reentrant modifier should be in the first place

L01 - Missing checks for address(0x0) when assigning values to address state variables

Mitigation

Add check for address(0x0)

Lines in the code

WardenPledge.sol#L137-L140

Non Critical

NC01 - Event is missing indexed fields

Description

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (threefields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

Lines in the code

WardenPledge.sol#L85-L119

NC02 - Outdated compiler version

Description

The project is using the solidity version 0.8.10. It's a best practice to use the latest release version. You can consult it in the following link

Mitigation

Update the solidity version to 0.8.17

Lines in the code

WardenPledge.sol#L2

NC03 - No reentrant modifier should be in the first place

WardenPledge.sol#L206 WardenPledge.sol#L307 WardenPledge.sol#L373 WardenPledge.sol#L419 WardenPledge.sol#L456 WardenPledge.sol#L488

Index

1. Using storage instead of memory for structs/arrays
2. I = I + (-) X is cheaper in gas cost than I += (-=) X
3. Emit the event before modify the storage variable
4. Operatos >= cost less gas than operator >
5. State variables that never change should be declared immutable or constant

Details

1. Using storage instead of memory for structs/arrays

Description

When retrieving data from a memory location, assigning the data to a memory variable causes all fields of the struct/array to be read from memory, resulting in a Gcoldsload (2100 gas) for each field of the struct/array. When reading fields from new memory variables, they cause an extra MLOAD instead of a cheap stack read. Rather than declaring a variable with the memory keyword, it is much cheaper to declare a variable with the storage keyword and cache all fields that need to be read again in a stack variable, because the fields actually read will only result in a Gcoldsload. The only case where the entire struct/array is read into a memory variable is when the entire struct/array is returned by a function, passed to a function that needs memory, or when the array/struct is read from another store array/struc

Lines in the code

WardenPledge.sol#L227

2. I = I + (-) X is cheaper in gas cost than I += (-=) X

Description

In the following example (optimizer = 10000) it's possible to demostrate that I = I + X cost less gas than I += X in state variable.

contract Test_Optimization {
    uint256 a = 1;
    function Add () external returns (uint256) {
        a = a + 1;
        return a;
    }
}

contract Test_Without_Optimization {
    uint256 a = 1;
    function Add () external returns (uint256) {
        a += 1;
        return a;
    }
}

• Test_Optimization cost is 26324 gas
• Test_Without_Optimization cost is 26440 gas

With this optimization it's possible to save 116 gas

Lines in the code

WardenPledge.sol#L268 WardenPledge.sol#L340 WardenPledge.sol#L401 WardenPledge.sol#L445

3. Emit the event before modify the storage variable

Description

If emit the event before assign de value to the local variable we can save to use the local variable and remove it.

    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);
    }

WardenPledge.sol#L599-L605

    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);
    }

WardenPledge.sol#L612-L618

    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);
    }

WardenPledge.sol#L625-L631

4. Operatos >= cost less gas than operator >

Description

When use >= the evm use only LT and when use > the evm use GT and ISZERO what allow to save 3 gas for each instance.

Lines in the code

WardenPledge.sol#L207 WardenPledge.sol#L234 WardenPledge.sol#L245 WardenPledge.sol#L267 WardenPledge.sol#L329 WardenPledge.sol#L330 WardenPledge.sol#L389 WardenPledge.sol#L390 WardenPledge.sol#L434 WardenPledge.sol#L435 WardenPledge.sol#L463 WardenPledge.sol#L666

5. State variables that never change should be declared immutable or constant

Description

Avoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).

Lines in the code

WardenPledge.sol#L60 WardenPledge.sol#L62