Badger Citadel contest - hubble's results

Summary of Findings for Low / Non-Critical issues

• L-01 : Critical null check missing during change of governance address
• L-02 : Lower and upper bounds not checked during updating rate value in function setEpochRate
• L-03 : Null check for _gac address missing in initialize() of StakedCitadelVester.sol
• L-04 : Restrict setting value for setEpochRate for epochs too far in future
• L-05 : Missing-zero-address-validation in multiple functions in SettAccessControl.sol
• L-06 : Input validation missing for duration in setVestingDuration

Details L-01

Title : Critical null check missing during change of governance address

Proof of Concept

Contract : SettAccessControl.sol

    function setGovernance(address _governance) public {
        _onlyGovernance();
        governance = _governance;
    }

Recommended Mitigation Steps

Add a null check for the governance address. Additionally as a best practice the two step standard process for changing the Governance address is recommended. 1st Step : set_new_governor << setting/proposing the address of the new_governor 2nd Step : accept_ownership << the new_governor will execute this function to complete the ownership transfer

Details L-02

Title : Lower and upper bounds not checked during updating rate value in function setEpochRate

Impact

The emission rate will be impacted if by mistake a rate is set too high or to 0. There is no check for uppper bound or lower bound for this rate parameter.

Proof of Concept

Contract : SupplySchedule.sol Function : function setEpochRate

Recommended Mitigation Steps

Define min and max values and check during setEpochRate

Details L-03

Title : Null check for _gac address missing in initialize() of StakedCitadelVester.sol

Proof of Concept

Contract : StakedCitadelVester.sol Function : initialize()

Recommended Mitigation Steps

Add a require statement to check null value for _gac address parameter

Details L-04

Title : Restrict setting value for setEpochRate for epochs too far in future

Its possible by the governance to set the EpochRate for epocs too far in the future, by mistake. Once set, the rate cannot be undone or changed.

Proof of Concept

Contract : SupplySchedule.sol
Function : setEpochRate(uint256 _epoch, uint256 _rate)

Recommended Mitigation Steps

Allow only setting for next few epocs, alternately set a max epoch in future for which the rates can be set.

Details L-05

Title : Missing-zero-address-validation in multiple functions in SettAccessControl.sol

The following functions setStrategist, setKeeper does not check for null values of address parameter.

SettAccessControl.setStrategist(address)._strategist (src/lib/SettAccessControl.sol#37) lacks a zero-check on : - strategist = _strategist (src/lib/SettAccessControl.sol#39) SettAccessControl.setKeeper(address)._keeper (src/lib/SettAccessControl.sol#44) lacks a zero-check on : - keeper = _keeper (src/lib/SettAccessControl.sol#46)

Recommended Mitigation Steps

Add a requrie statement to check null value for address parameter

Details L-06

Title : Input validation missing for duration in setVestingDuration

There is no check for the input value for the duration in function setVestingDuration in StakedCitadelVester.sol If wrongly set to 0 or a high value, then the vesting schedule will be hugely impacted.

Proof of Concept

Contract : StakedCitadelVester.sol

    function setVestingDuration(uint256 _duration)
        external
        onlyRole(CONTRACT_GOVERNANCE_ROLE)
    {
        vestingDuration = _duration;
        emit SetVestingDuration(_duration);
    }

Recommended Mitigation Steps

Add checks for min and max values of the duration value