`Validator::blacklist()` - Adding `address(0)` to the blacklist accidentally, which is totally possible, will temporarily DoS most user-facing protocol functionality.

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 0xSCSamurai Submission hash (on-chain): 0xb3d3f3ef3cf38c0682691b6c64f2d98625923ed71596a75d3db118c391f8826d Severity: medium

Description: Description\

• Since the BLACKLISTER_ROLE is trusted, if address(0) is accidentally added to the blacklist, which isn't an impossible scenario(mistakes happen), it will DoS most protocol functionality for users, luckily only temporarily until the WHITELISTER_ROLE removes the blacklist status from address(0) via calling the void() function.

• Although it's quite unlikely that the trusted BLACKLISTER_ROLE will accidentally add address(0) to the blacklist via the single address function, it becomes much more likely for such an accident/mistake to happen when adding multiple addresses to the blacklist via the multiple addresses blacklist() function, where an array list will be used, as there are no checks in either functions to prevent address(0) from being blacklisted, and it's totally possible for address(0) to accidentally sneak into a larger array list without detection...

• And since critical protocol functionality heavily relies on the use of address(0) for minting and burning transactions, the main protocol functionality, deposits/mints & withdrawals/redemptions, will be temporarily DoS-ed.

Much less likely for address(0) to accidentally sneak in here:

    function blacklist(address account) external onlyRole(BLACKLISTER_ROLE) {
        _blacklist(account);
    }

But:

Much more likely for address(0) to accidentally sneak in where an array list is used:

    function blacklist(address[] calldata accounts) external onlyRole(BLACKLISTER_ROLE) {
        for (uint256 i; i < accounts.length; i++) {
            _blacklist(accounts[i]);
        }
    }

IMPACT:

• Impact: high
• Likelihood: low-medium
• Severity: low-medium >>> low severity because address(0) can quickly be removed from the blacklist once the cause of the DoS reverts is discovered; medium severity because it's a massive disruption to the protocol, and if the cause of the major DoS reverts isn't found quickly, it could result in a serious disruption of primary protocol functionality.

Deeming this vulnerability as medium severity.

Proof of Concept (PoC Tests):

Test contract used, please ignore the generic code which I'm re-using for all my other tests:

// SPDX-License-Identifier: MIT
// SPDX-FileCopyrightText: © 2023 Rhinefield Technologies Limited

pragma solidity ^0.8.21;

import {Test} from "forge-std/Test.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
import {IUSDE} from "../src/interfaces/IUSDE.sol";
import {IYieldOracle} from "../src/interfaces/IYieldOracle.sol";
import {InvestToken} from "../src/InvestToken.sol";
import {USDE} from "../src/USDE.sol";
import {YieldOracle} from "../src/YieldOracle.sol";
import {Constants} from "./Constants.sol";
import "../src/Validator.sol";
import {console2} from "../lib/forge-std/src/console2.sol";

contract EuroDollarSetup is Test {
    Validator public validator;
    address public initialOracle;
    YieldOracle public yieldOracle;
    USDE public eud;
    IUSDE public iusde;
    InvestToken public eui;
    USDE eudImplementation;
    ERC1967Proxy eudProxy;
    ERC1967Proxy euiProxy;

    address public owner;

    address public whitelister;
    address public blacklister;
    address public pauser;

    uint256 public constant MIN_PRICE = 1e18; // Minimum EUI/USDE price
    bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;
    bytes32 public constant MINT_ROLE = keccak256("MINT_ROLE");
    bytes32 public constant BURN_ROLE = keccak256("BURN_ROLE");
    bytes32 public constant PAUSE_ROLE = keccak256("PAUSE_ROLE");
    bytes32 public constant FREEZE_ROLE = keccak256("FREEZE_ROLE");
    bytes32 public constant BLOCK_ROLE = keccak256("BLOCK_ROLE");
    bytes32 public constant ALLOW_ROLE = keccak256("ALLOW_ROLE");
    bytes32 public constant ORACLE_ROLE = keccak256("ORACLE_ROLE");
    bytes32 public constant WHITELISTER_ROLE = keccak256("WHITELISTER_ROLE");
    bytes32 public constant BLACKLISTER_ROLE = keccak256("BLACKLISTER_ROLE");
    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

    function setUp() public virtual {
        owner = address(this);
        initialOracle = address(0x3);

        whitelister = address(0x1);
        blacklister = address(0x2);
        pauser = address(0x5);
        validator = new Validator(owner, whitelister, blacklister);

        eudImplementation = new USDE(validator);
        eudProxy = new ERC1967Proxy(address(eudImplementation), abi.encodeCall(USDE.initialize, (owner)));
        eud = USDE(address(eudProxy));
        iusde = IUSDE(address(eudProxy));

        yieldOracle = new YieldOracle(owner, initialOracle);

        InvestToken euiImplementation = new InvestToken(validator, iusde);
        euiProxy =
        new ERC1967Proxy(address(euiImplementation), abi.encodeCall(InvestToken.initialize, ("EUI", "EUI", owner, yieldOracle)));

        eui = InvestToken(address(euiProxy));
        eui.grantRole(ALLOW_ROLE, owner);
        eui.grantRole(eui.RESCUER_ROLE(), owner);
        eud.grantRole(eud.RESCUER_ROLE(), owner);
        eui.grantRole(WHITELISTER_ROLE, whitelister);
        eui.grantRole(BLACKLISTER_ROLE, blacklister);
        validator.grantRole(BLACKLISTER_ROLE, blacklister);
        validator.grantRole(WHITELISTER_ROLE, whitelister);
        eui.grantRole(PAUSER_ROLE, pauser);

        deal(address(eui), address(this), 6999999); 
        deal(address(eui), address(eui), 1000000 * 2); 
        deal(address(eud), whitelister, 1000000);
        deal(address(eud), address(this), 1000000 * 20);
        deal(address(iusde), address(this), 1000000 * 10);
        deal(address(eudProxy), address(this), 1000000 * 10);

        eud.grantRole(eud.MINT_ROLE(), address(eui));
        eud.grantRole(eud.BURN_ROLE(), address(eui));

        eud.grantRole(eud.MINT_ROLE(), owner);

        vm.startPrank(whitelister);
        validator.whitelist(owner);
        validator.whitelist(whitelister);
        validator.whitelist(address(eui));
        vm.stopPrank();

        // vm.startPrank(blacklister);
        // //vm.expectRevert(); /// only uncomment this when the fix is activated...
        // validator.blacklist(address(0));
        // vm.stopPrank();
        // /// This successfully removes `address(0)` from the blacklist.
        // vm.startPrank(whitelister);
        // validator.void(address(0));
        // vm.stopPrank();
    }

    function testRecover() public { 
        vm.startPrank(owner);
        uint256 eui_shares = eui.deposit(1000000, address(this));
        bool success = eui.recover(owner, whitelister, 1000000);
        vm.stopPrank();

        vm.startPrank(owner);
        eui_shares = eui.deposit(1000000, address(this));
        vm.stopPrank();

        vm.startPrank(owner);
        eui_shares = eui.deposit(1000000, whitelister);
        vm.stopPrank();

        vm.startPrank(owner);
        eui_shares = eui.deposit(1000000, address(eui));
        success = eui.recover(address(eui), whitelister, 1000000);
        vm.stopPrank();

        eui.recover(owner, whitelister, 4000000);
        eui.recover(address(eui), whitelister, eui.balanceOf(address(eui)));

        vm.startPrank(whitelister);
        uint256 maxWithdraw = eui.maxWithdraw(owner);
        bool transferred = eui.transfer(owner, 3000001);
        vm.stopPrank();
        uint256 totalEudAssetsForSharesRedeemed = eui.redeem(maxWithdraw, owner, owner);
        vm.startPrank(whitelister);
        eud.transfer(owner, 1000000);
        vm.stopPrank();
        vm.startPrank(owner);
        eud.transfer(address(eui), 3999997);
        bool eudAssetsRecoveredFromOwnerAccount = eud.recover(owner, whitelister, eui.maxWithdraw(owner));
        bool eudAssetsRecoveredFromEuiVault = eud.recover(address(eui), whitelister, eui.maxWithdraw(address(eui))); 
        vm.stopPrank();

        console2.log("Did the recover transactions succeed? ", success);
        console2.log("Did the deposit transactions succeed? YES if they returned > 0 shares: ", eui_shares, " shares returned.");
        console2.log("Did the transfer transactions succeed? ", transferred);
    }
}

The only block of code from above test contract that will be adjusted/modified for each of the below PoC tests:

        // vm.startPrank(blacklister);
        // //vm.expectRevert(); /// only uncomment this when the fix is activated...
        // validator.blacklist(address(0));
        // vm.stopPrank();
        // /// This successfully removes `address(0)` from the blacklist.
        // vm.startPrank(whitelister);
        // validator.void(address(0));
        // vm.stopPrank();

Test 1: address(0) not blacklisted, normal expected behaviour:

Using:

        // vm.startPrank(blacklister);
        // //vm.expectRevert(); /// only uncomment this when the fix is activated...
        // validator.blacklist(address(0));
        // vm.stopPrank();
        // /// This successfully removes `address(0)` from the blacklist.
        // vm.startPrank(whitelister);
        // validator.void(address(0));
        // vm.stopPrank();

Test Result:

    ├─ [12070] ERC1967Proxy::recover(ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], ECRecover: [0x0000000000000000000000000000000000000001], 0)
    │   ├─ [11674] USDE::recover(ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], ECRecover: [0x0000000000000000000000000000000000000001], 0) [delegatecall]
    │   │   ├─ [1017] Validator::isValid(ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], 0x0000000000000000000000000000000000000000) [staticcall]
    │   │   │   └─ ← [Return] true
    │   │   ├─ emit Transfer(from: ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], to: 0x0000000000000000000000000000000000000000, value: 0)
    │   │   ├─ [1017] Validator::isValid(0x0000000000000000000000000000000000000000, ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
    │   │   │   └─ ← [Return] true
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: ECRecover: [0x0000000000000000000000000000000000000001], value: 0)
    │   │   ├─ emit Recovered(from: ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], to: ECRecover: [0x0000000000000000000000000000000000000001], amount: 0)
    │   │   └─ ← [Return] true
    │   └─ ← [Return] true
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    ├─ [0] console::log("Did the recover transactions succeed? ", true) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Did the deposit transactions succeed? YES if they returned > 0 shares: ", 1000000 [1e6], " shares returned.") [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Did the transfer transactions succeed? ", true) [staticcall]
    │   └─ ← [Stop] 
    └─ ← [Stop] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.97ms (479.96µs CPU time)

Logs: Did the recover transactions succeed? true Did the deposit transactions succeed? YES if they returned > 0 shares: 1000000 shares returned. Did the transfer transactions succeed? true

Test 2: address(0) is now blacklisted, everything reverts(deposits, recover, withdraw):

Using:

        vm.startPrank(blacklister);
        //vm.expectRevert(); /// only uncomment this when the fix is activated...
        validator.blacklist(address(0));
        vm.stopPrank();
        // /// This successfully removes `address(0)` from the blacklist.
        // vm.startPrank(whitelister);
        // validator.void(address(0));
        // vm.stopPrank();

Test Result:

   ├─ [0] VM::startPrank(SHA-256: [0x0000000000000000000000000000000000000002])
    │   └─ ← [Return] 
    ├─ [24172] Validator::blacklist(0x0000000000000000000000000000000000000000)
    │   ├─ emit Blacklisted(account: 0x0000000000000000000000000000000000000000)
    │   └─ ← [Stop] 
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    └─ ← [Stop] 

  [41744] EuroDollarSetup::testRecover()
    ├─ [0] VM::startPrank(EuroDollarSetup: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← [Return] 
    ├─ [31655] ERC1967Proxy::deposit(1000000 [1e6], EuroDollarSetup: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   ├─ [26752] InvestToken::deposit(1000000 [1e6], EuroDollarSetup: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) [delegatecall]
    │   │   ├─ [2643] YieldOracle::assetsToShares(1000000 [1e6]) [staticcall]
    │   │   │   └─ ← [Return] 1000000 [1e6]
    │   │   ├─ [15896] ERC1967Proxy::burn(EuroDollarSetup: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 1000000 [1e6])
    │   │   │   ├─ [10993] USDE::burn(EuroDollarSetup: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 1000000 [1e6]) [delegatecall]
    │   │   │   │   ├─ [5017] Validator::isValid(EuroDollarSetup: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x0000000000000000000000000000000000000000) [staticcall]
    │   │   │   │   │   └─ ← [Return] false
    │   │   │   │   └─ ← [Revert] revert: account blocked
    │   │   │   └─ ← [Revert] revert: account blocked
    │   │   └─ ← [Revert] revert: account blocked
    │   └─ ← [Revert] revert: account blocked
    └─ ← [Revert] revert: account blocked

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 6.68ms (207.91µs CPU time)

Ran 1 test suite in 1.17s (6.68ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/InvestToken_report_test1.sol:EuroDollarSetup
[FAIL. Reason: revert: account blocked] testRecover() (gas: 41744)

• Blacklisted address(0) causes most of the critical user facing functions to fail due to account blocked reverts.

Test 3: address(0) removed from the blacklist by whitelister, and now everything works as expected again:

Using:

        vm.startPrank(blacklister);
        //vm.expectRevert(); /// only uncomment this when the fix is activated...
        validator.blacklist(address(0));
        vm.stopPrank();
        /// This successfully removes `address(0)` from the blacklist.
        vm.startPrank(whitelister);
        validator.void(address(0));
        vm.stopPrank();

Test Result:

    ├─ [12070] ERC1967Proxy::recover(ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], ECRecover: [0x0000000000000000000000000000000000000001], 0)
    │   ├─ [11674] USDE::recover(ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], ECRecover: [0x0000000000000000000000000000000000000001], 0) [delegatecall]
    │   │   ├─ [1017] Validator::isValid(ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], 0x0000000000000000000000000000000000000000) [staticcall]
    │   │   │   └─ ← [Return] true
    │   │   ├─ emit Transfer(from: ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], to: 0x0000000000000000000000000000000000000000, value: 0)
    │   │   ├─ [1017] Validator::isValid(0x0000000000000000000000000000000000000000, ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
    │   │   │   └─ ← [Return] true
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: ECRecover: [0x0000000000000000000000000000000000000001], value: 0)
    │   │   ├─ emit Recovered(from: ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], to: ECRecover: [0x0000000000000000000000000000000000000001], amount: 0)
    │   │   └─ ← [Return] true
    │   └─ ← [Return] true
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    ├─ [0] console::log("Did the recover transactions succeed? ", true) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Did the deposit transactions succeed? YES if they returned > 0 shares: ", 1000000 [1e6], " shares returned.") [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Did the transfer transactions succeed? ", true) [staticcall]
    │   └─ ← [Stop] 
    └─ ← [Stop] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.78ms (1.12ms CPU time)

Logs: Did the recover transactions succeed? true Did the deposit transactions succeed? YES if they returned > 0 shares: 1000000 shares returned. Did the transfer transactions succeed? true

Test 4: Mitigation fix implemented. Now address(0) cannot be added to the blacklist at all:

Note: For this test I used the suggested mitigation fix that I mention at the bottom of the report after PoC tests...

Using:

        vm.startPrank(blacklister);
        vm.expectRevert(); /// only uncomment this when the fix is activated...
        validator.blacklist(address(0));
        vm.stopPrank();
        // /// This successfully removes `address(0)` from the blacklist.
        // vm.startPrank(whitelister);
        // validator.void(address(0));
        // vm.stopPrank();

Test Result:

    ├─ [12070] ERC1967Proxy::recover(ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], ECRecover: [0x0000000000000000000000000000000000000001], 0)
    │   ├─ [11674] USDE::recover(ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], ECRecover: [0x0000000000000000000000000000000000000001], 0) [delegatecall]
    │   │   ├─ [1017] Validator::isValid(ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], 0x0000000000000000000000000000000000000000) [staticcall]
    │   │   │   └─ ← [Return] true
    │   │   ├─ emit Transfer(from: ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], to: 0x0000000000000000000000000000000000000000, value: 0)
    │   │   ├─ [1017] Validator::isValid(0x0000000000000000000000000000000000000000, ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
    │   │   │   └─ ← [Return] true
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: ECRecover: [0x0000000000000000000000000000000000000001], value: 0)
    │   │   ├─ emit Recovered(from: ERC1967Proxy: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], to: ECRecover: [0x0000000000000000000000000000000000000001], amount: 0)
    │   │   └─ ← [Return] true
    │   └─ ← [Return] true
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    ├─ [0] console::log("Did the recover transactions succeed? ", true) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Did the deposit transactions succeed? YES if they returned > 0 shares: ", 1000000 [1e6], " shares returned.") [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Did the transfer transactions succeed? ", true) [staticcall]
    │   └─ ← [Stop] 
    └─ ← [Stop] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.48ms (496.66µs CPU time)

• The fix solved the problem. Cannot add address(0) to the blacklist accidentally or intentionally.

  Logs: Did the recover transactions succeed? true Did the deposit transactions succeed? YES if they returned > 0 shares: 1000000 shares returned. Did the transfer transactions succeed? true

Suggested mitigation fix for this vulnerability:

    function blacklist(address account) external onlyRole(BLACKLISTER_ROLE) {
+       require(account != address(0), "Cannot blacklist zero address!");
        _blacklist(account);
    }

and

    function blacklist(address[] calldata accounts) external onlyRole(BLACKLISTER_ROLE) {
        for (uint256 i; i < accounts.length; i++) {
+           require(account[i] != address(0), "Cannot blacklist zero address!");
            _blacklist(accounts[i]);
        }
    }

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Files:

• InvestToken_report_test1.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmRMc2PRgzKkpeuqad4TQDvdDJZZT8MXYJSPuL1fDpUPeq)

[dappconsulting]

Please note, this bug report explains a vulnerability that does NOT require a malicious blacklister, but simply requires an accidental blacklisting of the address(0).

Specifically, the array list function for adding multiple addresses to the blacklist. It's arguably entirely possible for a mistake to be made and accidentally include address(0) in that list.

Furthermore, blacklisting address(0) has critical but temporary DoS impact on most of the primary user facing functionality of the protocol. So it isn't simply a little issue, it's arguably a major issue(DoS of user deposits/withdrawals), which can luckily be fixed quickly but only once the protocol team has figured out what the problem was, which in this case is an accidentally blacklisted address(0).

Unless there's a solid valid reason for intentionally ALLOWING for address(0) to be blacklisted, I argue that a mitigation fix should be implemented to completely eliminate this vulnerability.

thanks