Paladin - Warden Pledges contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

Owner can rug all pledges

Proof of Concept

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

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

Owner can rug all pledge creators by delisting reward token to remove it's protection then calling recoverERC20 to take the entire balance. In the event that the owner wallet is compromised all user funds would be stolen.

Tools Used

Manual Review

Recommended Mitigation Steps

Delisted assets should be added to a mapping. When calling recoverERC20, it should revert if mapping returns true for that asset. This protects users funds even after a reward token is delisted.

Lines of code

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

Vulnerability details

Impact

Delisted reward tokens can continue to be use by extending current pledges that already use it

Proof of Concept

if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID();
address creator = pledgeOwner[pledgeId];
if(msg.sender != creator) revert Errors.NotPledgeCreator();


Pledge storage pledgeParams = pledges[pledgeId];
if(pledgeParams.closed) revert Errors.PledgeClosed();
if(pledgeParams.endTimestamp <= block.timestamp) revert Errors.ExpiredPledge();
if(newEndTimestamp == 0) revert Errors.NullEndTimestamp();
uint256 oldEndTimestamp = pledgeParams.endTimestamp;
if(newEndTimestamp != _getRoundedTimestamp(newEndTimestamp) || newEndTimestamp < oldEndTimestamp) revert Errors.InvalidEndTimestamp();


uint256 addedDuration = newEndTimestamp - oldEndTimestamp;
if(addedDuration < minDelegationTime) revert Errors.DurationTooShort();
uint256 totalRewardAmount = (pledgeParams.rewardPerVote * pledgeParams.votesDifference * addedDuration) / UNIT;
uint256 feeAmount = (totalRewardAmount * protocalFeeRatio) / MAX_PCT ;
if(totalRewardAmount > maxTotalRewardAmount) revert Errors.IncorrectMaxTotalRewardAmount();
if(feeAmount > maxFeeAmount) revert Errors.IncorrectMaxFeeAmount();

During the input validation checks, it's never checked that reward token of the pledge being extended is still a valid reward token. This would allow creators using delisted tokens to continue using them as long as they wanted, by simply extending their currently active pledges.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the following check during the input validation block:

+   if(minAmountRewardToken[rewardToken] == 0) revert Errors.TokenNotWhitelisted();

Lines of code

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

Vulnerability details

Impact

Owner may accidentally transfer ownership to inoperable address due to perceived safeguard that doesn't exist

Proof of Concept

contract WardenPledge is Ownable, Pausable, ReentrancyGuard {

WardenPledge inherits from Ownable rather than Owner, which is the intended contract. Owner overwrites the critical Ownable#transferOwnership function to make the ownership transfer process a two step process. This adds important safeguards because in the event that the target is unable to accept for any reason (input typo, incompatible multisig/contract, etc.) the ownership transfer process will fail because the pending owner will not be able to accept the transfer. To make matters worse, since it only overwrites the trasnferOwnership function the WardenPledge contract will otherwise function as intended just without this safeguard. It is likely that the owner won't even realize until too late and the safeguard has failed. A perceived safeguard where there isn't one is more damaging than not having any safeguard at all.

Tools Used

Manual Review

Recommended Mitigation Steps

-   contract WardenPledge is Ownable, Pausable, ReentrancyGuard {
+   contract WardenPledge is Owner, Pausable, ReentrancyGuard {

Lines of code

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

Vulnerability details

Impact

Pledge creator is overcharged fees

Proof of Concept

vars.duration = endTimestamp - block.timestamp;
if(vars.duration < minDelegationTime) revert Errors.DurationTooShort();

vars.votesDifference = targetVotes - votingEscrow.balanceOf(receiver);

vars.totalRewardAmount = (rewardPerVote * vars.votesDifference * vars.duration) / UNIT;
vars.feeAmount = (vars.totalRewardAmount * protocalFeeRatio) / MAX_PCT ;

The amount of voting power added by a boost decays rapidly and is calculated by the second, making it highly unlikely that the entire reward amount will be used up. To make the issue worse, current boost aren't counted when calculating the total boost amount needed, meaning that if the target of the pledge already has a boost, the creator is overcharged to a greater extent. Since the remaining amount is returned to the user after the pledge is complete, taking the full reward amount is fine. The fee however is taken in full based on the optimistic amount calculated which is almost certainly too much, which means the creator is overcharged.

Tools Used

Manual Review

Recommended Mitigation Steps

The full amount of the pledge fee should be sent directly to the pledge contract. The pledge fee should be calculated and applied when each boost is received and the amount should be claimable. After a pledge is over, the pledge creator should receive their refund amount along with all unused fees.

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L299-L358 https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L368-L404

Vulnerability details

Impact

Setting protocol fee to 0 will break support for reward tokens that don't support 0 transfers

Proof of Concept

IERC20(rewardToken).safeTransferFrom(creator, address(this), totalRewardAmount);
IERC20(rewardToken).safeTransferFrom(creator, chestAddress, feeAmount);

WardenPledge#increasePledgeRewardPerVote, extendPledge and createPledge will all attempt to send a protocol fee even if there is not protocol fee to collect. This is problematic as some ERC20 tokens revert when transferring an amount of 0.

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

    emit PlatformFeeUpdated(oldfee, newFee);
}

updatePlatformFee allows owner to set protocol fee to 0. In this situation pledges that use affected reward tokens can no longer be created, extended or have their rewards increased.

Tools Used

Manual Review

Recommended Mitigation Steps

    IERC20(rewardToken).safeTransferFrom(creator, address(this), totalRewardAmount);
+   if(feeAmount != 0) {
+       IERC20(rewardToken).safeTransferFrom(creator, chestAddress, feeAmount);
+   }