Nibbl contest - berndartmueller's results

QA Report

Table of Contents

• Low Risk
  • [L-01] chainid() may change in case of a hardfork
  • [L-02] Unrecoverable ETH due to unnecessary receive() function

Low Risk

[L-01] chainid() may change in case of a hardfork

Description

The domainSeperator is not recalculated in the case of a hard fork. The variable domainSeperator in the contract EIP712Base is cached in the contract storage and will not change after being initialized. However, in the event of a hard fork, the domain would become invalid on one of the forked chains due to the change of chainid.

Findings

EIP712Base.sol#L16-L24

function INIT_EIP712(string memory name, string memory version) internal {
    domainSeperator = keccak256(
        abi.encode(
            EIP712_DOMAIN_TYPEHASH,
            keccak256(bytes(name)),
            keccak256(bytes(version)),
            getChainID(),
            address(this)
        )
    ); // @audit-info `domainSeperator` is cached forever
}

Recommended mitigation steps

Consider using the elegant solution provided by OpenZeppelin:

https://github.com/fractional-company/contracts/blob/master/src/OpenZeppelin/drafts/EIP712.sol

[L-02] Unrecoverable ETH due to unnecessary receive() function

Description

The NibblVault contract does not expect direct ETH transfers. However, there is a receive() function to receive ETH. Anyone accidentally sending ETH to this contract will lose their funds.

Findings

NibblVault.sol#L585

receive() external payable {}

Recommended mitigation steps

Remove the receive() function.