Paladin - Warden Pledges contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

There is a recoverERC20() function to withdraw ERC20 tokens from the contract.

Currently, it checks if the token isn't an active reward token but it can be passed easily if the admin removes the reward token using removeRewardToken().

So if the admin removes an active reward token and withdraws all amount of the token from the contract, the pledge creator using that token will lose their funds permanently.

Even if the admin doesn't do that by fault, it's a potential admin attack vector to steal all user funds.

Proof of Concept

• As we can see, recoverERC20() can withdraw the entire balance of the token if it's not an active reward 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;
    }

• The admin can remove tokens from reward token using removeRewardToken() even if there is an active pledge using this token.

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

• So if the admin removes the reward token and calls recoverERC20() by fault or maliciously to steal funds, all pledge creators who used this token will lose their funds.

Tools Used

Manual Review

Recommended Mitigation Steps

I think we should add an additional mapping for the reward token like activePledgeCount to count the number of active pledges to use the token and update the mapping properly when users create/close the pledges.

And we can check if activePledgeCount[token] == 0 in the removeRewardToken() so that the admin can't remove the reward token that has an active pledge.

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

        minAmountRewardToken[token] = 0;
        
        emit RemoveRewardToken(token);
    }

Lines of code

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

Vulnerability details

Impact

Currently, the pledge creators can't do anything after the protocol is paused.

So they can't withdraw their unused funds after the pledge is expired in the pause mode and the funds will be locked in the contract.

Proof of Concept

• As we can see from retrievePledgeRewards() and closePledge, both functions have a whenNotPaused modifier.

    function retrievePledgeRewards(uint256 pledgeId, address receiver) external whenNotPaused nonReentrant {
        if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID();
        address creator = pledgeOwner[pledgeId];
        if(msg.sender != creator) revert Errors.NotPledgeCreator();
        if(receiver == address(0)) revert Errors.ZeroAddress();

        Pledge storage pledgeParams = pledges[pledgeId];
        if(pledgeParams.endTimestamp > block.timestamp) revert Errors.PledgeNotExpired();

        // Get the current remaining amount of rewards not distributed for the Pledge
        uint256 remainingAmount = pledgeAvailableRewardAmounts[pledgeId];

        // Set the Pledge as Closed
        if(!pledgeParams.closed) pledgeParams.closed = true;

        if(remainingAmount > 0) {
            // Transfer the non used rewards and reset storage
            pledgeAvailableRewardAmounts[pledgeId] = 0;

            IERC20(pledgeParams.rewardToken).safeTransfer(receiver, remainingAmount);

            emit RetrievedPledgeRewards(pledgeId, receiver, remainingAmount);

        }
    }

    /**
    * @notice Closes a Pledge and retrieves all non distributed rewards from a Pledge
    * @dev Closes a Pledge and retrieves all non distributed rewards from a Pledge & send them to the given receiver
    * @param pledgeId ID fo the Pledge to close
    * @param receiver Address to receive the remaining rewards
    */
    function closePledge(uint256 pledgeId, address receiver) external whenNotPaused nonReentrant {
        if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID();
        address creator = pledgeOwner[pledgeId];
        if(msg.sender != creator) revert Errors.NotPledgeCreator();
        if(receiver == address(0)) revert Errors.ZeroAddress();

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

        // Set the Pledge as Closed
        pledgeParams.closed = true;

        // Get the current remaining amount of rewards not distributed for the Pledge
        uint256 remainingAmount = pledgeAvailableRewardAmounts[pledgeId];

        if(remainingAmount > 0) {
            // Transfer the non used rewards and reset storage
            pledgeAvailableRewardAmounts[pledgeId] = 0;

            IERC20(pledgeParams.rewardToken).safeTransfer(receiver, remainingAmount);

            emit RetrievedPledgeRewards(pledgeId, receiver, remainingAmount);

        }

        emit ClosePledge(pledgeId);
    }

• So creators can't withdraw their unused funds in the pause mode.
• From my experience working with other protocols, it's normal to allow creators to withdraw their unused funds after the pledge is finished even in the pause mode.
• Otherwise, the creator's funds might be locked inside the contract until the pause state is changed.

Tools Used

Manual Review

Recommended Mitigation Steps

I think retrievePledgeRewards() and closePledge() shouldn't have a whenNotPaused modifier.

If the protocol wants to add a strict requirement for closePledge(), at least retrievePledgeRewards() shouldn't have a modifier so that the creators can recover their funds after the pledge is expired.