Brickken Code Style Findings

BRI-01C: Ambiguous Overriding Declarations

Type	Severity	Location
Language Specific		Brickken.sol:L71, L78

Description:

The _mint and _burn functions which are overridden by the Brickken contract do not strictly specify which parent contract function should be executed first which can cause ambiguities in execution should the order of declaration in inheritence change as Solidity utilizes a similar algorithm to the C3 Linearization method of Python.

Example:

Copy
56/**
57 * @dev Hook called after each token transfer. It moves votes delegation
58 * calling the `ERC20Votes` function.
59 */
60function _afterTokenTransfer(address from, address to, uint256 amount)
61    internal
62    override(ERC20, ERC20Votes)
63{
64    ERC20Votes._afterTokenTransfer(from, to, amount);
65}
66
67function _mint(address to, uint256 amount)
68    internal
69    override(ERC20, ERC20Votes)
70{
71    super._mint(to, amount);
72}
73
74function _burn(address account, uint256 amount)
75    internal
76    override(ERC20, ERC20Votes)
77{
78    super._burn(account, amount);
79}

Recommendation:

We advise the super keyword to be replaced by the ERC20Votes specifier similarly to the _afterTokenTransfer function overriddence, thereby preventing any ambiguities from ever arising.

Alleviation:

The Brickken team has acknowledged this issue, however, they have opted to not apply an alleviation for it as they desire to minimize changes to their code since their contract has already been deployed.

BRI-02C: Duplicate Getter Function Call

Type	Severity	Location
Gas Optimization		Brickken.sol:L41, L43

Description:

The linked getter function is invoked twice in between statements its result is expected to remain the same.

Example:

Copy
40function burnFrom(address account, uint256 amount) public onlyRole(BURNER_ROLE){
41    uint256 currentAllowance = allowance(account, _msgSender());
42    require(currentAllowance >= amount, "ERC20: burn amount exceeds allowance");
43    _approve(account, _msgSender(), currentAllowance - amount);
44    _burn(account, amount);
45}

Recommendation:

We advise the result of _msgSender to be cached to a local variable that is consequently utilized for the allowance and _approve function calls optimizing the function's overall gas cost.

Alleviation:

The Brickken team has acknowledged this issue and will apply it in future iterations, however, they have opted to not apply an alleviation for it as they desire to minimize changes to their code since their contract has already been deployed.

BRI-03C: Duplicate Specification of Inheritence

Type	Severity	Location
Standard Conformity		Brickken.sol:L7, L8, L10

Description:

The Brickken contract inherits from both the ERC20Permit and ERC20Votes contracts, however, ERC20Votes itself inherits from ERC20Permit as evident here.

Example:

Copy
7import "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";
8import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol";
9
10contract Brickken is ERC20, AccessControl, ERC20Permit, ERC20Votes {

Recommendation:

We advise the import and usage of ERC20Permit to be omitted from the codebase as it is ineffectual.

Alleviation:

The Brickken team has stated that they prefer verbose import statements as their programming style and that the compiler is meant to be responsible for resolving import hierarchies. Given that this is the programming style the Brickken team desires to retain, we consider this exhibit nullified.

BRI-04C: Mathematical Optimization

Type	Severity	Location
Gas Optimization		Brickken.sol:L43

Description:

The linked subtraction between currentAllowance and amount is guaranteed to succeed by the require check that precedes it.

Example:

Copy
40function burnFrom(address account, uint256 amount) public onlyRole(BURNER_ROLE){
41    uint256 currentAllowance = allowance(account, _msgSender());
42    require(currentAllowance >= amount, "ERC20: burn amount exceeds allowance");
43    _approve(account, _msgSender(), currentAllowance - amount);
44    _burn(account, amount);
45}

Recommendation:

We advise the statement to be wrapped in an unchecked code block thereby optimizing its gas cost.

Alleviation:

The Brickken team has acknowledged this issue and will apply it in future iterations, however, they have opted to not apply an alleviation for it as they desire to minimize changes to their code since their contract has already been deployed.