Badger Citadel contest - 0x1f8b's results

1. OPEN TODO, it's a bad practice to have unfinished code during a security review.

• Funding.sol#61
• Funding.sol#183
• KnightingRound.sol#14
• GlobalAccessControl.sol#50
• GlobalAccessControl.sol#106
• SupplySchedule.sol#159

2. Unsecure Ownership Transfer

The modification process of an owner is a delicate process, since the governance of our contract and therefore of the project may be at risk, for this reason it is recommended to adjust the owner’s modification logic, to a logic that allows to verify that the new owner is in fact valid and does exist. It's mandatory to create a logic of the owner’s modification where a new owner is proposed first, the owner accepts the proposal and, in this way, we make sure that there are no errors when writing the address of the new owner.

• GlobalAccessControl.sol#61
• SettAccessControl.sol#53

2. Lack of input checks. Is not checked that the address is a contract distinct to address(0).

• SettAccessControl.sol#39
• SettAccessControl.sol#46

1. There are require messages bigger than 32 bytes. More than 32 bytes for message will incur an extra gas costs.

• StakedCitadel.sol#632
• StakedCitadel.sol#652
• Funding.sol#61
• Funding.sol#148
• Funding.sol#298
• Funding.sol#325
• Funding.sol#390
• KnightingRound.sol#122
• KnightingRound.sol#126
• KnightingRound.sol#130
• KnightingRound.sol#134
• KnightingRound.sol#273
• KnightingRound.sol#277
• KnightingRound.sol#295
• KnightingRound.sol#297
• KnightingRound.sol#314
• KnightingRound.sol#316
• KnightingRound.sol#333
• KnightingRound.sol#351
• KnightingRound.sol#384
• CitadelMinter.sol#301
• CitadelMinter.sol#321
• CitadelMinter.sol#328
• CitadelMinter.sol#370
• CitadelMinter.sol#377
• GlobalAccessControl.sol#118
• SupplySchedule.sol#62
• SupplySchedule.sol#92
• SupplySchedule.sol#96
• SupplySchedule.sol#139
• SupplySchedule.sol#143
• SupplySchedule.sol#157
• SupplySchedule.sol#181
• SupplySchedule.sol#185
• SupplySchedule.sol#189

2. It's compared a boolean value using == true or == false, instead of using the boolean value, or NOT opcode, it's cheaper to use NOT when the value it's false, or just the value without == true, when it's true, because it will use less opcode inside the VM.

• Funding.sol#147

3. Change the incremental logic from i++ to ++i in order to save some opcodes:

• CitadelMinter.sol#152
• CitadelMinter.sol#344
• SupplySchedule.sol#208
• GlobalAccessControlManaged#48

4. Avoid require when found in GlobalAccessControlManaged.sol#51

    modifier onlyRoles(bytes32[] memory roles) {
        uint256 rl = roles.length;
        for (uint256 i = 0; i < rl; ++i) {
            bytes32 role = roles[i];
            if (gac.hasRole(role, msg.sender)) {
                _;
                return;
            }
        }
        revert("GAC: invalid-caller-role");
    }

5. Invert condition in GlobalAccessControlManaged.sol.onlyRoleOrAddress because it will be very cheaper if it was the address and not the role

require(
-           gac.hasRole(role, msg.sender) || msg.sender == account,
+           msg.sender == account || gac.hasRole(role, msg.sender),
            "GAC: invalid-caller-role-or-address"
        );