Paladin - Warden Pledges contest - horsefacts's results

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L652-L661

Vulnerability details

The recoverERC20 function allows the contract owner to transfer arbitrary ERC20 tokens owned by the WardenPledge contract in order to recover tokens sent by mistake to the contract. In order to protect against withdrawal of deposited reward tokens, it includes a check that the withdrawn token is not currently an approved rewards token:

recoverERC20

/**
 * @notice Recovers ERC2O tokens sent by mistake to the contract
 * @dev Recovers ERC2O tokens sent by mistake to the contract
 * @param token Address tof the EC2O token
 * @return bool: success
 */
function recoverERC20(address token) external onlyOwner returns (bool) {
  if (minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken();

  uint256 amount = IERC20(token).balanceOf(address(this));
  if (amount == 0) revert Errors.NullValue();
  IERC20(token).safeTransfer(owner(), amount);

  return true;
}

However, the contract owner also has the ability to remove rewards tokens, and can easily bypass the check in recoverERC20 by first calling removeRewardToken to set minAmountRewardToken[token] to zero, then calling recoverERC20:

removeRewardToken

function removeRewardToken(address token) external onlyOwner {
  if (token == address(0)) revert Errors.ZeroAddress();
  if (minAmountRewardToken[token] == 0) revert Errors.NotAllowedToken();

  minAmountRewardToken[token] = 0;

  emit RemoveRewardToken(token);
}

Impact: A malicious owner can steal all deposited rewards tokens.

Recommendation: Disallow removing rewards tokens associated with active pledges. This will probably require tracking this information in a separately stored data structure.

QA

Low

Pledges do not support fee on transfer tokens

The total amount required to fund a pledge is calculated at pledge creation time, then transferred to the WardenPledge contract:

createPledge

        vars.totalRewardAmount = (rewardPerVote * vars.votesDifference * vars.duration) / UNIT;
        vars.feeAmount = (vars.totalRewardAmount * protocalFeeRatio) / MAX_PCT ;
        if(vars.totalRewardAmount > maxTotalRewardAmount) revert Errors.IncorrectMaxTotalRewardAmount();
        if(vars.feeAmount > maxFeeAmount) revert Errors.IncorrectMaxFeeAmount();

        // Pull all the rewards in this contract
        IERC20(rewardToken).safeTransferFrom(creator, address(this), vars.totalRewardAmount);

The total reward amount is then stored, along with the reward rate inside the Pledge struct:

createPledge

        // Add the total reards as available for the Pledge & write Pledge parameters in storage
        pledgeAvailableRewardAmounts[vars.newPledgeID] += vars.totalRewardAmount;

        pledges.push(Pledge(
            targetVotes,
            vars.votesDifference,
            rewardPerVote,
            rewardToken,
            safe64(endTimestamp),
            false,
            receiver
        ));

However, if a pledge is created with a fee-on-transfer token, an amount less than the full calculated reward amount will be transferred to the WardenPledge contract when the pledge is created, but the full amount will be stored for reward calculations. The contract may show that there are sufficient rewards remaining for additional pledges, but revert when a pledger actually attempts to make the pledge due to an insufficient balance.

I consider this low risk, since all rewards tokens must be approved before use, and it is unlikely that rewards would be paid in a fee-on-transfer token, but ensure you qualify all rewards tokens before approval.

QA

Use date units

The constant WEEK can be defined using time unit keywords, either 7 days or 1 weeks. (These keywords are used elsewhere in the contract).

WardenPledge.sol:

    uint256 public constant WEEK = 7 * 86400;

    uint256 public constant WEEK = 1 weeks;

Incomplete comment

There is an incomplete comment on line 84 of WardenPledge:

WardenPledge#L84

    /** @notice Event emitted when xx */

Index NewPledge parameters

Consider indexing the address parameters of NewPledge, to allow filtering events by creator/receiver/rewardToken address:

WardenPledge#L85:

    event NewPledge(
        address creator,
        address receiver,
        address rewardToken,
        uint256 targetVotes,
        uint256 rewardPerVote,
        uint256 endTimestamp
    );

Misspelling

"Protocol fee ratio" is misspelled as "protocal fee ratio":

WardenPledge#L627

       uint256 oldfee = protocalFeeRatio;
        protocalFeeRatio = newFee;

Just passing along two small findings in this report.

Declare variables immutable

The votingEscrow and delegationBoost state variables are set once at construction time and cannot change, so you can declare them immutable:

WardenPledge#L59

    /** @notice Address of the votingToken to delegate */
    IVotingEscrow public votingEscrow;
    /** @notice Address of the Delegation Boost contract */
    IBoostV2 public delegationBoost;

Suggestion:

    /** @notice Address of the votingToken to delegate */
    IVotingEscrow public immutable votingEscrow;
    /** @notice Address of the Delegation Boost contract */
    IBoostV2 public immutable delegationBoost;

Use unchecked subtraction

Since the if statement on line 267 ensures that rewardAmount is greater than pledgeAvailableRewardAmounts[pledgeId], you may safely perform an unchecked subtraction:

_pledge

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

Suggested:

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