Badger Citadel contest - Picodes's results

L-01 - Add constructor with initializer in upgradeable contracts

Description

All upgradeable contracts of the repo should implement a void constructor to call the initialized flag to true in the implementation.

Citing other audit reports: "As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.” Furthermore, this thwarts any attempts to frontrun the initialization tx of the implementation contract. Incorporating this change would require inheriting the Initializable contract instead of having an explicit initialized variable."

Recommended Mitigation Steps

Implementation contracts should inherit OZ’s Initializable contract and have the following constructor method:

import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contract A is 
    Initializable
    ...
{
    ...
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() initializer {
        // so that users won't accidentally send JPYC to the implementation contract
        blocklisted[address(this)] = true;
    }
}

L-02 - Useless variable in Global Access Control

Variable transferFromDisabled is not used in the code and should be removed from the contract.

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/GlobalAccessControl.sol#L51

NC-01 - Token could implement EIP-2612

For integration purposes and UX experience, team should consider implementing https://eips.ethereum.org/EIPS/eip-2612 for its ERC-20 token, especially CitadelToken