Paladin - Warden Pledges contest - minhtrng'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

There is a function that is intended to be used to recover ERC20 tokens that were sent to the WardenPledge contract by accident. The function is only usable by the owner and contains a check that no tokens can be taken which are currently whitelisted as reward tokens. However, the owner also has enough privileges to bypass that check and can instantly take all reward tokens that are part of currently running pledges.

Proof of Concept

The function to move out accidentally sent ERC20 tokens as described under #Impact is 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;
}

It can be seen that it reverts if minAmountRewardToken[token] != 0, which means the token can be used during the creation of a new pledge and also that there might be currently pledges running with that token as a reward.

However, the owner can use the removeRewardToken function at will to "de-whitelist" any such tokens, which would allow him to sweep all reward tokens with recoverERC20 right after:

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

Tools Used

Manual Review

Recommended Mitigation Steps

• Option 1: make the sweep of tokens a 2-step process where the second step is timelocked and the first step emits an event to notify users that a sweep is about to happen for a certain token. This gives users the time and opportunity to react accordingly.
• Option 2: track all currently running pledges for a certain token and only enable recovery for that token if there are no corresponding pledges running.

Lines of code

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

Vulnerability details

Impact

The accounting for the reward token amount currently assumes that the WardenPledge contract will have a balance equal to an amount x if a transferFrom for said amount x happens to the contract. If an ERC20 token gets whitelisted that has a fee-on-transfer mechanism and is subsequently used for a pledge, the accounting will be incorrect and the creator of the pledge will not be able to close the pledge or withdraw the remaining tokens.

Proof of Concept

In the function createPledge the amount that is assigned to pledgeAvailableRewardAmounts will be the same as was was requested in a call to rewardtoken.safeTransferFrom:

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

vars.newPledgeID = pledgesIndex();

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

In case of rewardToken having fees-on-transfer, the actual value by which the balance of reward tokens has increased on the WardenPledge contract will be lower than that. This can make the functions retrievePledgeRewards and closePledge uncallable if there is only one currently running pledge with that token as a reward, as they both try to perform to transfer more tokens than the contract currently holds:

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

In case of multiple running pledges the first pledge creators to call either function will effectively steal tokens from other pledges.

Tools Used

Manual Review

Recommended Mitigation Steps

• Option 1: Track the value of the balanceOf function of the reward token before and after a safeTransferFrom and assign the difference to pledgeAvailableRewardAmounts[vars.newPledgeID]
• Option 2: ensure through some (offchain-)means that no fee-on-transfer tokens will ever get whitelisted, if the system is not meant to be compliant with them.