Paladin - Warden Pledges contest - wagmi's results

Lines of code

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

Vulnerability details

Impact

Function recoverERC20(...) can be used to recover ERC20 tokens sent by mistake to the contract. It has a check that the ERC20 token is not whitelisted to prevent admin from withdrawing funds of users.

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, admin can use another function removeRewardToken(...) to remove token from whitelist then call recoverERC20(...). The function removeRewardToken(...) does not have any check if there is any pledge with this token existing. This way, admin can rug pull and drain all funds of users in this contract.

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

Proof of Concept

Consider the scenario

1. Admin whitelisted USDT as reward token.
2. Alice (creator) created a pledge and transfer 100k USDT to WardenPledge contract
3. Admin can call removeRewardToken(...) and recoverERC20(...) to drain 100k USDT from contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding the mechanism that only allowing admin remove reward token when there is no pledge with that token active.

For example, adding a mapping count[token] that store number of active pledge with that token.