Paladin - Warden Pledges contest - rvierdiiev's results

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.

1. Use solidity variable instead of calculations

No need to calculate how much is 1 week in seconds. You can use solidity 1 weeks variable instead. uint256 public constant WEEK = 1 weeks; https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L24

2. Use constant instead of magic number

Use constant instead of magic number 500 inside updatePlatformFee function. https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L626

3. WardenPledge can be initialized with 0 chest address which leads to unpredictable behaviour of protocol.

In constructor of WardenPledge there is no check that chest address is not 0. However it is in setter. chest address is used in different places of code to transfer protocol fees to it using IERC20(rewardToken).safeTransferFrom(creator, chestAddress, vars.feeAmount);. If chest value was initialized as 0 using constructor then the behaviour of protocol will depend on reward token. Different reward tokens will give different results. Some will revert, because of sending to 0 address, while someone will still work. As the result, in one option protocol will be working with reward token, but will burn protocol fees and in another option, protocol will revert when sending fees.

Make sure that chest != address(0) in constructor.