Paladin - Warden Pledges contest - carlitox477's results

constructor does not check address != zero for input

It allows to set votingEscrow, delegationBoost and chestAddress to address zero.

Impact:

• Setting votingEscrow to address zero will leave pledgePercent and createPledge unusable.
• Setting delegationBoost to address zero will leave _pledge unusable. This will provoke functions pledge and pledgePercent to become unusable too.

WardenPledge#pledgesIndex is so simple that can be declared as external

This function just returns pledges.length. This is simple to do inside the contract, calling this function inside the contract is more expensive than just calling pledges.length.

Mitigation steps: replace every occurency of pledgesIndex() for pledges.length; and change pledgesIndex()visibility to external

updateRewardToken can emit invalid event due to lack of check

If minAmountRewardToken[token] == minRewardPerSecond then the minAmountRewardToken[token] would not really be updated, leading to a wrong UpdateRewardToken event emission.

Mitigation steps: Check that minAmountRewardToken[token] != minRewardPerSecond

updateChest can emit invalid event due to lack of check

If chestAddress == chest then the chestAddress would not really be updated, leading to a wrong ChestUpdated event emission

Mitigation steps: Check that chestAddress != chest

updateMinTargetVotes can emit invalid event due to lack of check

If minTargetVotes == newMinTargetVotes then the minTargetVotes would not really be updated, leading to a wrong MinTargetUpdated event emission

Mitigation steps: Check that minTargetVotes != newMinTargetVotes

updatePlatformFee can emit invalid event due to lack of check

If protocalFeeRatio == newFee then the protocalFeeRatio would not really be updated, leading to a wrong PlatformFeeUpdated event emission

Mitigation steps: Check that protocalFeeRatio != newFee

minDelegationTime can be declared as a private constant

It is not modifed anywhere

_pledge can save gas if first we check amount

The amount check do just one comparison and no function call, so amount == 0 is cheaper than pledgeId >= pledgesIndex(). Replace these line for:

if(amount == 0) revert Errors.NullValue();
if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID();

_pledge function call pledgesIndex() can be omitted

Calling pledgesIndex() is more expensive than just calling pledges.length. Replace this line for

if(pledgeId >= pledges.length) revert Errors.InvalidPledgeID();

_pledge can cache delegationBoost

This state variable is used multiple times, it can be cached in order to save gas.

_pledgeAvailableRewardAmounts = pledgeAvailableRewardAmounts[pledgeId]
if(rewardAmount > _pledgeAvailableRewardAmounts) revert Errors.RewardsBalanceTooLow();
pledgeAvailableRewardAmounts[pledgeId] = _pledgeAvailableRewardAmounts - rewardAmount;

createPledge call pledgesIndex() can be omitted

This state variable is used multiple times, it can be cached in order to save gas.

// vars.newPledgeID = pledgesIndex(); //before
vars.newPledgeID = plendge.length();

createPledge double access to state variable minAmountRewardToken[rewardToken] can be omitted

This state variable is accessed two times, it can be cached in order to save gas.

if(receiver == address(0) || rewardToken == address(0)) revert Errors.ZeroAddress();
if(targetVotes < minTargetVotes) revert Errors.TargetVoteUnderMin();

uint256 _minAmountRewardToken = minAmountRewardToken[rewardToken];
if(_minAmountRewardToken == 0) revert Errors.TokenNotWhitelisted();
if(rewardPerVote < _minAmountRewardToken) revert Errors.RewardPerVoteTooLow();

extendPledge call pledgesIndex() can be omitted

Calling pledgesIndex() is more expensive than just calling pledges.length. Replace this line for

if(pledgeId >= pledges.length) revert Errors.InvalidPledgeID();

extendPledge: pledgeParams.rewardToken is accessed twice

Replace these lines for:

address _rewardToken = pledgeParams.rewardToken
// Pull all the rewards in this contract
IERC20(_rewardToken).safeTransferFrom(creator, address(this), totalRewardAmount);
// And transfer the fees from the Pledge creator to the Chest contract
IERC20(_rewardToken).safeTransferFrom(creator, chestAddress, feeAmount);

increasePledgeRewardPerVote call pledgesIndex() can be omitted

Calling pledgesIndex() is more expensive than just calling pledges.length. Replace this line for

if(pledgeId >= pledges.length) revert Errors.InvalidPledgeID();

increasePledgeRewardPerVote: pledgeParams.rewardToken is accessed twice

Replace these lines for:

address _rewardToken = pledgeParams.rewardToken
// Pull all the rewards in this contract
IERC20(_rewardToken).safeTransferFrom(creator, address(this), totalRewardAmount);
// And transfer the fees from the Pledge creator to the Chest contract
IERC20(_rewardToken).safeTransferFrom(creator, chestAddress, feeAmount);

retrievePledgeRewards call pledgesIndex() can be omitted

Calling pledgesIndex() is more expensive than just calling pledges.length. Replace this line for

if(pledgeId >= pledges.length) revert Errors.InvalidPledgeID();

closePledge call pledgesIndex() can be omitted

Calling pledgesIndex() is more expensive than just calling pledges.length. Replace this line for

if(pledgeId >= pledges.length) revert Errors.InvalidPledgeID();

increasePledgeRewardPerVote: can use unchecked block for newRewardPerVote - oldRewardPerVote

Given that newRewardPerVote <= oldRewardPerVote was previously check, it is impossible for newRewardPerVote - oldRewardPerVote to underflow. Replace this line for

unchecked{
    uint256 rewardPerVoteDiff = newRewardPerVote - oldRewardPerVote;
}

increasePledgeRewardPerVote: can use unchecked block for pledgeParams.endTimestamp - block.timestamp

Given that pledgeParams.endTimestamp <= block.timestamp was previously check, it is impossible for pledgeParams.endTimestamp - block.timestamp to underflow. Replace this line for

unchecked{
    uint256 remainingDuration = pledgeParams.endTimestamp - block.timestamp;
}

increasePledgeRewardPerVote: can use unchecked block for pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;

Given that this amount was previously transfered, if this transfer was successful we can assure that it won't be an overflow, replace this line for:

unchecked{
    pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;
}

extendPledge: can use unchecked block for newEndTimestamp - oldEndTimestamp calculation

Possible underflow was previously checked. Replace this line for

unchecked{
    uint256 addedDuration = newEndTimestamp - oldEndTimestamp;
}

extendPledge: can use unchecked block for pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;

Given that this amount was previously transfered, if this transfer was successful we can assure that it won't be an overflow, replace this line for:

unchecked{
    pledgeAvailableRewardAmounts[pledgeId] += totalRewardAmount;
}

createPledge: can use unchecked block for pledgeAvailableRewardAmounts[vars.newPledgeID] += vars.totalRewardAmount;

Given that this amount was previously transfered, if this transfer was succesful we can assure that it won't be an overflow, replace this line for:

unchecked{
    pledgeAvailableRewardAmounts[vars.newPledgeID] += vars.totalRewardAmount;
}

_pledge: can use unchecked block for pledgeAvailableRewardAmounts[pledgeId] -= rewardAmount;

Given that this amount was previously checked, we can assure that it won't be an underflow, replace this line for:

unchecked{
    pledgeAvailableRewardAmounts[pledgeId] -= rewardAmount;
}