Malicious or hacked admin can drain all tokens

[code423n4]

Lines of code

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

Vulnerability details

Impact

Hacked or malicious admin can transfer all tokens of WardenPledge to himself.

Proof of Concept

WardenPledge.recoverERC20 function allows to transfer tokens controlled by contract if the token is not whitelisted.

    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 check if(minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken(); is done to not allow admin to send tokens that are used for sending rewards to himself.
But there is another function that allows admin to remove token from whitelisted.

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

Using this function admin can first remove token from whitelisted and then recover tokens to himself.

I believe this is important thing and protocol should secure this. Because even if admin is not malicious, but ownership somehow was hacked, this gives ability for the attacker to get all funds and pausing can't help here.

Tools Used

VsCode

Recommended Mitigation Steps

You can disallow to blocklist token if the WardenPledge balance is not 0.

[Kogaroshi]

PoC of the Issue is correct, but Remediation brings another issue in the system: in the case of a malicious or bugged token that was whitelisted, and currently in use for a Pledge, that needs to be removed from the whitelist, this would be impossible because the contract balance won't be 0.

Will look for an in-between that could prevent the scenario doing most of the damages to the system.

[Kogaroshi]

Fixed in PR 2, commit
Decided on a different way to fix this:

• tracking all token amounts currently belonging to Pledges
• change the recoverERC20() method to ignore the whitelist, but forbidding the method to touch any token belonging to Pledges.

That way, admin has no way to touch any Pledge rewards or rug any funds, even after removing a token from the whitelist.
The new system also allows to retrieve the reward token sent directly to the contract by mistake, without touching any Pledge rewards.

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird marked the issue as primary issue

[c4-judge]

Simon-Busch marked the issue as duplicate of #68