Paladin - Warden Pledges contest - __141345__'s results

Event is missing indexed fields

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 (three fields). 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.

Instances number of this issue: 6

85-92
    event NewPledge(
        address creator,
        address receiver,
        address rewardToken,
        uint256 targetVotes,
        uint256 rewardPerVote,
        uint256 endTimestamp
    );
94     event ExtendPledgeDuration(uint256 indexed pledgeId, uint256 oldEndTimestamp, uint256 newEndTimestamp);
96     event IncreasePledgeTargetVotes(uint256 indexed pledgeId, uint256 oldTargetVotes, uint256 newTargetVotes);
98     event IncreasePledgeRewardPerVote(uint256 indexed pledgeId, uint256 oldRewardPerVote, uint256 newRewardPerVote);
102    event RetrievedPledgeRewards(uint256 indexed pledgeId, address receiver, uint256 amount);
105    event Pledged(uint256 indexed pledgeId, address indexed user, uint256 amount, uint256 endTimestamp);

Missing Equivalence Checks in Setters

Description: Setter functions are missing checks to validate if the new value being set is the same as the current value already set in the contract. Such checks will showcase mismatches between on-chain and off-chain states.

Suggestion: This may hinder detecting discrepancies between on-chain and off-chain states leading to flawed assumptions of on-chain state and protocol behavior.

Instances number of this issue: 3

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

        emit ChestUpdated(oldChest, chest);
    }

    function updateMinTargetVotes(uint256 newMinTargetVotes) external onlyOwner {
        if(newMinTargetVotes == 0) revert Errors.InvalidValue();
        uint256 oldMinTarget = minTargetVotes;
        minTargetVotes = newMinTargetVotes;

        emit MinTargetUpdated(oldMinTarget, newMinTargetVotes);
    }

    function updatePlatformFee(uint256 newFee) external onlyOwner {
        if(newFee > 500) revert Errors.InvalidValue();
        uint256 oldfee = protocalFeeRatio;
        protocalFeeRatio = newFee;

        emit PlatformFeeUpdated(oldfee, newFee);
    }

Missing Time locks

When critical parameters of systems need to be changed, it is required to broadcast the change via event emission and recommended to enforce the changes after a time-delay. This is to allow system users to be aware of such critical changes and give them an opportunity to exit or adjust their engagement with the system accordingly. None of the onlyOwner functions that change critical protocol addresses/parameters have a timelock for a time-delayed change to alert:

• users and give them a chance to engage/exit protocol if they are not agreeable to the changes
• team in case of compromised owner(s) and give them a chance to perform incident response.

Instances number of this issue: 3

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

        emit ChestUpdated(oldChest, chest);
    }

    function updateMinTargetVotes(uint256 newMinTargetVotes) external onlyOwner {
        if(newMinTargetVotes == 0) revert Errors.InvalidValue();
        uint256 oldMinTarget = minTargetVotes;
        minTargetVotes = newMinTargetVotes;

        emit MinTargetUpdated(oldMinTarget, newMinTargetVotes);
    }

    function updatePlatformFee(uint256 newFee) external onlyOwner {
        if(newFee > 500) revert Errors.InvalidValue();
        uint256 oldfee = protocalFeeRatio;
        protocalFeeRatio = newFee;

        emit PlatformFeeUpdated(oldfee, newFee);
    }

Suggestion: Users may be surprised when critical parameters are changed. Furthermore, it can erode users' trust since they can’t be sure the protocol rules won’t be changed later on. Compromised owner keys may be used to change protocol addresses/parameters to benefit attackers. Without a time-delay, authorized owners have no time for any planned incident response. (May be medium since owner can change critical parameters at anytime that can affect the users poorly).

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

Instances number of this issue:

268        pledgeAvailableRewardAmounts[pledgeId] -= rewardAmount;

340        pledgeAvailableRewardAmounts[vars.newPledgeID] += vars.totalRewardAmount;
401        pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;
445        pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;

Update value order can be adjusted to simplify the code and save gas

For example, to update the num variable with newVal, the current way is as following:

    uint oldVal = num;
    num = newVal;
    emit update(oldVal, newVal);

If the execution order is adjusted, some operations can be saved (memory space allocation, variable assignment), reducing both the deployment and run time gas cost.

    emit update(num, newVal);
    num = newVal;

The operation saved: 1 sload, 1 memory allocation, 1 mstore.

The demo of the gas comparison can be seen here.

There are multiple places can use this trick for optimization, since the updates of parameters are widely and frequently used, the optimization can be beneficial.

Instances number of this issue: 3

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

        emit ChestUpdated(oldChest, chest);
    }

    function updateMinTargetVotes(uint256 newMinTargetVotes) external onlyOwner {
        if(newMinTargetVotes == 0) revert Errors.InvalidValue();
        uint256 oldMinTarget = minTargetVotes;
        minTargetVotes = newMinTargetVotes;

        emit MinTargetUpdated(oldMinTarget, newMinTargetVotes);
    }

    function updatePlatformFee(uint256 newFee) external onlyOwner {
        if(newFee > 500) revert Errors.InvalidValue();
        uint256 oldfee = protocalFeeRatio;
        protocalFeeRatio = newFee;

        emit PlatformFeeUpdated(oldfee, newFee);
    }

Code reuse

Some function codes overlap. Consider reuse the common part and save deployment gas.

• retrievePledgeRewards() and closePledge() codes overlap a lot, only a few lines of difference.

• the following are used in extendPledge(), increasePledgeRewardPerVote() and _pledge():

        Pledge storage pledgeParams = pledges[pledgeId];
        if(pledgeParams.closed) revert Errors.PledgeClosed();
        if(pledgeParams.endTimestamp <= block.timestamp) revert Errors.ExpiredPledge();

pledgeParams.closed could be deleted

This variable does not seem useful, since the check for if(pledgeParams.endTimestamp <= block.timestamp) can serve the purpose of pledgeParams.closed.

Consider delete pledgeParams.closed.