Removing or never setting blockList causes reverts

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L124-L130

Vulnerability details

Unset blocklist will cause reverts to most functions.

While it may seem natural to have a blocklist setup at this time, the constructor doesn't take it as an argument.

Additionally, in the future, the DAO may vote to remove the blocklist, however, an unset blockList causes Reverts as the modifier always runs the call to the contract, and setting blockList to address(0) (or never setting it), will cause reverts.

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L124-L130

    modifier checkBlocklist() {
        require(
            !IBlocklist(blocklist).isBlocked(msg.sender),
            "Blocked contract"
        );
        _;
    }

POC

I encountered this error when trying to build a POC for another attack, see steps below

contract = accountsBrownie environment is ready.
>>> contract = accounts[0]
>>> puppet = accounts[1]
>>> token = MockERC20.deploy("mock", "MOCK", {"from": a[0]})
Transaction sent: 0xbbd0d39421c9894f87b85bc871326d378c2210b58cbb448ccec307e8525235ea
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 4
  MockERC20.constructor confirmed   Block: 15347217   Gas used: 653229 (5.44%)
  MockERC20 deployed at: 0xe0aA552A10d7EC8760Fc6c246D391E698a82dDf9

>>> token.mint(contract, 1e20, {"from": a[0]})
Transaction sent: 0x57ceb3944c5d4a8bec15ecfc670cd5cab3c8340de645a44d2ea070f040e14ad3
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 5
  MockERC20.mint confirmed   Block: 15347218   Gas used: 65760 (0.55%)

<Transaction '0x57ceb3944c5d4a8bec15ecfc670cd5cab3c8340de645a44d2ea070f040e14ad3'>
>>> token.mint(puppet, 1, {"from": a[0]})
Transaction sent: 0xed69ffa223e0c41bf7e000aa8f4ece37be3dfab18ed35fcfc5c511d79c753f09
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 6
  MockERC20.mint confirmed   Block: 15347219   Gas used: 50688 (0.42%)

<Transaction '0xed69ffa223e0c41bf7e000aa8f4ece37be3dfab18ed35fcfc5c511d79c753f09'>
>>> owner = a[2]
>>> ve = VotingEscrow.deploy(owner, owner, token, "ve", "VE", {"from": a[2]})
Transaction sent: 0xac16e01acdee66a605eb89216ca507c5139622c2be61ad9c70fd909c41173840
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 0
  VotingEscrow.constructor confirmed   Block: 15347220   Gas used: 3554365 (29.62%)
  VotingEscrow deployed at: 0x1596Ff8ED308a83897a731F3C1e814B19E11D68c

>>> token.approve(ve, 1, {"from": puppet})
Transaction sent: 0x5e85517c643952492a1806fd27b478ceb6662c746dd614492ee06a74e7c4ff37
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 5
  MockERC20.approve confirmed   Block: 15347221   Gas used: 44096 (0.37%)

<Transaction '0x5e85517c643952492a1806fd27b478ceb6662c746dd614492ee06a74e7c4ff37'>
>>> token.approve(ve, token.balanceOf(contract), {"from": contract})
Transaction sent: 0xb29376bd8b6f7b4ff9f477aa5d3e02d161e165fcb17126bd6d218827a14ceee7
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 7
  MockERC20.approve confirmed   Block: 15347222   Gas used: 44168 (0.37%)

<Transaction '0xb29376bd8b6f7b4ff9f477aa5d3e02d161e165fcb17126bd6d218827a14ceee7'>
>>> ve.createLock(1, chain.time() + ve.MAXTIME() - 1000, {"from": puppet})
Transaction sent: 0x479e9d795112024c9281fce79c9291c4e476b0ac62f92ad2ea6a3062dc75b6bb
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 6
  VotingEscrow.createLock confirmed (reverted)   Block: 15347223   Gas used: 29311 (0.24%)

<Transaction '0x479e9d795112024c9281fce79c9291c4e476b0ac62f92ad2ea6a3062dc75b6bb'>

>>> history[-1].error()
Trace step 170, program counter 9415:
  File "contracts/VotingEscrow.sol", line 126, in VotingEscrow.createLock:    

    modifier checkBlocklist() {
        require(
            !IBlocklist(blocklist).isBlocked(msg.sender),
            "Blocked contract"
        );
        _;

All functions won't work until blocklist is set.

Remediation Steps

Add a check for blocklist being zero, and skip the call

    modifier checkBlocklist() {
        address cachedList = blocklist; // Cache to save gas if set
        if (cachedList != address(0)) {
            require(
                !IBlocklist(cachedList).isBlocked(msg.sender),
                "Blocked contract"
            );
        }
        _;

Additionally you could add initialBlocklist as a constructor argument

[lacoop6tu]

Expected behaviour

[elnilz]

veFDT will always used in combination with a blocklist contract

we don't initialize blocklist in the constructor bc blocklist itself is initialized with the address of veFDT. so veFDT is deployed first but only accepts deposits once a blocklist is initialized

[gititGoro]

The reverted calls is enough signal that the blocklist hasn't been deployed. Adding new code to handle an incomplete deploy script penalizes the users' gas at the benefit for the deployer.