Staking Code Style Findings

STA-01C: Illegible Declaration Style

Description:

The StakingSchedule struct is located at the IStaking.sol file which is separate, thus causing order-based declarations to be hard to decipher.

Example:

Copy
59function initialize(IERC20 _tokeToken, IManager _manager, address _treasury) public initializer {
60    __Context_init_unchained();
61    __Ownable_init_unchained();
62    tokeToken = _tokeToken;        
63    manager = _manager;
64    nextScheduleIndex = 0;
65    treasury = _treasury;
66
67    //We want to be sure the schedule used for LP staking is first
68    //because the order in which withdraws happen need to start with LP stakes
69    _addSchedule(StakingSchedule(0,1,1,true,true,0,true));
70
71}

Recommendation:

We advise the key-value style of declaration to be applied instead, greatly increasing the legibility of the linked statement.

Alleviation:

The key-value style is now properly utilized in the _addSchedule invocation.

STA-02C: Misused Delete Statement

Description:

The delete operation in Solidity simply zeroes out the data space of the specified variable.

Example:

Copy
217if (requestedWithdrawals[msg.sender].amount == 0) {
218    delete requestedWithdrawals[msg.sender];
219}

Recommendation:

We advise the delete statement to be replaced by an assignment of 0 to minCycleIndex as it currently preforms one redundant storage write.

Alleviation:

The Tokemak team stated that the delete instruction better illustrates the purpose of the statement and decided to leave the code as-is.

STA-03C: Redundant Assignment

Description:

All variables in Solidity are initialized to their default value on declaration which is mostly equivalent to the value of 0 in the respective type.

Example:

Copy
59function initialize(IERC20 _tokeToken, IManager _manager, address _treasury) public initializer {
60    __Context_init_unchained();
61    __Ownable_init_unchained();
62    tokeToken = _tokeToken;        
63    manager = _manager;
64    nextScheduleIndex = 0;
65    treasury = _treasury;
66
67    //We want to be sure the schedule used for LP staking is first
68    //because the order in which withdraws happen need to start with LP stakes
69    _addSchedule(StakingSchedule(0,1,1,true,true,0,true));
70
71}

Recommendation:

We recommend the linked assignments to be omitted as they are ineffectual.

Alleviation:

Only the first of the three redundant withdrawals was omitted from the codebase.

STA-04C: Redundant System Implementation

Description:

The scheduleIdxs is meant to hold all IDs that have been set to date, however, it is entirely redundant given that the system uses sequentially incremented IDs.

Example:

Copy
310function _addSchedule(StakingSchedule memory schedule) private  {
311    require(schedule.duration > 0, "INVALID_DURATION");
312    require(schedule.interval > 0, "INVALID_INTERVAL");
313
314    schedule.setup = true;        
315    uint256 index = nextScheduleIndex;
316    schedules[index] = schedule;
317    scheduleIdxs.add(index);
318    nextScheduleIndex = nextScheduleIndex.add(1);
319    
320    emit ScheduleAdded(index, schedule.cliff, schedule.duration, schedule.interval, schedule.setup, schedule.isActive, schedule.hardStart);
321}

Recommendation:

We advise the nextScheduleIndex variable to be utilized instead of scheduleIdxs.length(), the actual value of the iterator i to be used instead of scheduleIdxs.at(i) and finally a validation of schedules[scheduleIndex].setup instead of scheduleIdxs.contains(scheduleIndex). These adaptations will render the system redundant and greatly reduce the gas cost of the contract across the board.

Alleviation:

The Tokemak team responded by stating that the schedule indexes are utilized to reduce looping in order to retrieve all schedules and that it should be retained in the codebase.

STA-05C: Simplification of Statements

Description:

The if-else branch within setPermissionedDepositor is redundant.

Example:

Copy
77function setPermissionedDepositor(address account, bool canDeposit) external override onlyOwner {
78    if (canDeposit) {
79        permissionedDepositors[account] = canDeposit;
80    } else {
81        delete permissionedDepositors[account];
82    }
83}

Recommendation:

We advise a direct assignment of canDeposit to be done to permissionedDepositors[account] as the delete operation is equivalent to setting the variable to false.

Alleviation:

The if-else structure was replaced properly by a direct assignment.