Paladin - Warden Pledges contest - 0xDecorativePineapple's results

Lines of code

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

Vulnerability details

Impact

The owner of the WardenPledge.sol smart contract can steal all the reward tokens from the contract and break the internal accounting. With the recoverERC20 function, the owner can transfer to him/herself the whole balance of the token. The check at L654 (if(minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken();) can be bypassed by first calling the removeRewardToken function and setting the if(minAmountRewardToken[token] to zero.

This will also break the internal accounting, as multiple functions of the smart contract, like closePledge and retrievePledgeRewards will break.

Proof of Concept

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 owner calls to steal the whole balance of the tokenA
• (S)he calls the removeRewardToken function with the address of the tokenA and sets the value of the minAmountRewardToken[token] mapping to zero
• Then, (s)he calls the recoverERC20 function with the address of the tokenA and transfers to herself the whole balance of the tokenA

Tools Used

Manual code review

Recommended Mitigation Steps

It is recommended to delete the removeRewardToken function or to create a mapping that tracks the balance of the reward token that is transferred to the smart contract, and then let the owner to only call the recoverERC20 function with the difference amount: uint256 amount = IERC20(token).balanceOf(address(this)) - amountOfToken[token];