code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/features/Blocklist.sol#L25

Vulnerability details

[M-00] Attacker contract can avoid being blocked by BlockList.sol

Problem

To block an address it must pass the isContract(address) check: https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/features/Blocklist.sol#L25

contracts/features/Blocklist.sol
25:         require(_isContract(addr), "Only contracts");

Which just checks code length at the address provided.

contracts/features/Blocklist.sol
37:     function _isContract(address addr) internal view returns (bool) {
38:         uint256 size;
39:         assembly {
40:             size := extcodesize(addr)
41:         }
42:         return size > 0;
43:     }

Attacker can interact with the system and selfdestruct his contract, and with help of CREATE2 recreate it at same address when he needs to interact with the system again.

Proof of concept

Below is a simple example of salted contract creation, which you can test against _isContract(address) function.

pragma solidity 0.8.15;

contract BlockList {
    function _isContract(address addr) external view returns (bool) {
        uint256 size;
        assembly {
            size := extcodesize(addr)
        }
        return size > 0;
    }
}

contract AttackerContract {
  function destroy() external {
    selfdestruct(payable(0));
  }
}

contract AttackerFactory {
    function deploy() external returns (address) {
        return address(new AttackerContract{salt: bytes32("123")}());
    }
}

Mitigation

One of the goals of Ethereum is for humans and smart contracts to both be treated equally. This leads into a future where smart contracts interact seamlessly with humans and other contracts. It might change in the future , but for now an arbitrary address is ambiguous. We should consider blacklisting addresses without checking if they are contracts.

bahurum commented 2 years ago

Duplicate of #168

lacoop6tu commented 2 years ago

Duplicate of #168

gititGoro commented 2 years ago

This is a valid attack vector that undermines the blocking mechanism and is not a duplicate of #168.

lacoop6tu commented 2 years ago

imo this is more acknowledged in this case, the only interaction possible is locking LP tokens first so if someone then selfdestructs, another attacker could create a contract with that address and take ownership (and quitLock for example) similar to what happened with optimism and wintermute