Paladin - Warden Pledges contest - imare's results

Lines of code

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

Vulnerability details

Impact

The owner of a WardenPledge contract instance can recover stuck ERC20 token using recoverERC20. recoverERC20 will revert in case a token is used as a reward token.

But if the owner prior of calling recoverERC20 calls removeRewardToken he/she can take any ERC20 token out of the contract.

This is not good for investors. To give more trust to users these functions should be put behind a timelock.

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

Add a timelock with an on-chain governance controlling the smart contract at least for recoverERC20 function.

[QA-01] Use pagination for getAllPledges() function

This function returns array of all Pledge structures which is a fairly big structure. I think this is a good case for using pagination. Something like this:

function getAllPledges(uint256 cursor, uint256 howMany) 
    external view 
    returns(Pledge[] memory values, uint256 newCursor) {
            
        uint256 length = howMany;
        if (length > pledges.length - cursor) {
            length = pledges.length - cursor;
        }

        values = new Pledge[](length);
        for (uint256 i; i < length; ) {
            values[i] = pledges[cursor + i];

            unchecked{ ++i; }
        }

        return (values, cursor + length);
}

[QA-02] Can remove WEEK constant state variable

Solidity provides some native units for dealing with time. The WEEK constant its only used in one place when calculating the rounded down timestamp.

Instead of using this :

    uint256 public constant WEEK = 7 * 86400;

    function _getRoundedTimestamp(uint256 timestamp) view public returns(uint256) {
        return (timestamp / WEEK) * WEEK;
    }

You can use something like this:

function _getRoundedTimestamp(uint256 timestamp) view public returns(uint256) {
        return (timestamp / 1 weeks) * 1 weeks;
    }

[LOW-01] retrievePledgeRewards is not doing what the comment says is doing

The comment for this functions says that the function will : Retrieves all non distributed rewards from a closed Pledge But the function

1. is not verifying a Pledge is closed and then retrieving the rewards
2. but is closing a Pledge and then retrieving rewards (this action can be repeated)

The only difference with closePledge function is that the closePledge will check if a Pledge is closed and revert in the case it is closed.

I don't think you need both functions because a user cannot pledge a closed Pledge so there is no point to having both. And also in closing the pledge with closePledge you are also retrieving the rewards as in (as the name would suggest) retrievePledgeRewards.

[G-01] Functions guaranteed to revert when called by normal users can be marked payable

If a function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 10 instances of this issue:

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

    function addMultipleRewardToken(address[] calldata tokens, uint256[] calldata minRewardsPerSecond) external onlyOwner {

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

    function addRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {

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

    function updateRewardToken(address token, uint256 minRewardPerSecond) external onlyOwner {

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

function removeRewardToken(address token) external onlyOwner {

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

    function updateChest(address chest) external onlyOwner {

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

    function updateMinTargetVotes(uint256 newMinTargetVotes) external onlyOwner {

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

    function updatePlatformFee(uint256 newFee) external onlyOwner {

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

    function pause() external onlyOwner {

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

    function unpause() external onlyOwner {

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

    function recoverERC20(address token) external onlyOwner returns(bool) {