Paladin - Warden Pledges contest - Dravee's results

Lines of code

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

Vulnerability details

Impact

Admin can rug all of the contract's funds

Proof of Concept

The function recoverERC20() is only callable by the owner and its goal is: @notice Recovers ERC2O tokens sent by mistake to the contract. The call fails if minAmountRewardToken[token] != 0 , which is the check made to make sure that what's being recovered isn't a whitelisted token.

However, the following can be done in 1 transaction:

1. The owner calls removeRewardToken() to set minAmountRewardToken = 0 on any whitelisted token:

File: WardenPledge.sol
585:     function removeRewardToken(address token) external onlyOwner {
586:         if(token == address(0)) revert Errors.ZeroAddress();
587:         if(minAmountRewardToken[token] == 0) revert Errors.NotAllowedToken();
588:         
589:         minAmountRewardToken[token] = 0;
590:         
591:         emit RemoveRewardToken(token);
592:     }

2. The owner calls recoverERC20() on the un-whitelisted token to get the funds:

File: WardenPledge.sol
653:     function recoverERC20(address token) external onlyOwner returns(bool) {
654:         if(minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken();
655: 
656:         uint256 amount = IERC20(token).balanceOf(address(this));
657:         if(amount == 0) revert Errors.NullValue();
658:         IERC20(token).safeTransfer(owner(), amount);
659: 
660:         return true;
661:     }

Notice that a line such as this suggests that the contract does own whitelisted tokens:

File: WardenPledge.sol
332:         // Pull all the rewards in this contract
333:         IERC20(rewardToken).safeTransferFrom(creator, address(this), vars.totalRewardAmount);

3. The owner repeats 1) & 2) on all whitelisted tokens

4. The owner runs away

Recommended Mitigation Steps

The following balance check would prevent the above rug scenario but could be grieved by someone transferring even 1 token to the contract:

File: WardenPledge.sol
585:     function removeRewardToken(address token) external onlyOwner {
586:         if(token == address(0)) revert Errors.ZeroAddress();
587:         if(minAmountRewardToken[token] == 0) revert Errors.NotAllowedToken();
588:         
+ 588:         if(IERC20(token).balanceOf(address(this)) > 0) revert Errors.NotAllowedToken();
589:         minAmountRewardToken[token] = 0;
590:         
591:         emit RemoveRewardToken(token);
592:     }

Therefore, I believe the best solution would be putting in place an internal accounting and use the internal balance to check that a reward token can be removed.

1. Low Risk Issues

1.1. chestAddress can be set to 0 in the constructor and DOS createPledge()

There's no sanity-check to prevent a mistake such as setting chestAddress = 0 in the constructor. Notice that the check does exist in updateChest(). If chestAddress == 0, the following line in createPledge() would revert for some tokens such as USDC, which would prevent pledges from being created until the owner reacts to the mistake.

1.2. minTargetVotes can be set to 0 in the constructor and DOS createPledge()

Similar to above, minTargetVotes = 0 is possible in the constructor, while the check does exist in updateMinTargetVotes(). If minTargetVotes == 0, the following require statement will always fail in createPledge(), which would prevent pledges from being created until the owner reacts to the mistake.

2. Non-Critical Issue

2.1. pledgesIndex() should be renamed as nextPledgeID()

According to the documentation, pledgesIndex() is the Amount of Pledges listed in this contract, but the name is hacky (actually, pledgesIndex() returning pledges.length is returning a non-existing (yet) index, used to check the upper bound of indexes or to get the nextPledgeId). While its use is perfect in the code, it'd be better named as nextPledgeID() or pledgesLength

1. State variables only set in the constructor should be declared immutable

Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)

contracts/WardenPledge.sol:
   60:     IVotingEscrow public votingEscrow;//@audit-issue GAS should be immutable
   62:     IBoostV2 public delegationBoost;//@audit-issue GAS should be immutable
   79:     uint256 public minDelegationTime = 1 weeks; //@audit-issue GAS should be immutable

2. Use = instead of += when assigning a value for the 1st time

On the following, mapping(uint256 => uint256) public pledgeAvailableRewardAmounts is being set for the first time with the new key vars.newPledgeID:

File: WardenPledge.sol
337:         vars.newPledgeID = pledgesIndex();
338: 
339:         // Add the total reards as available for the Pledge & write Pledge parameters in storage
- 340:         pledgeAvailableRewardAmounts[vars.newPledgeID] += vars.totalRewardAmount; //@audit-issue GAS: it's a new var on a new ID
+ 340:         pledgeAvailableRewardAmounts[vars.newPledgeID] = vars.totalRewardAmount;

Therefore, += should be replaced with = to avoid reading the storage variable pledgeAvailableRewardAmounts[vars.newPledgeID] which is always equal to 0.