Omniscia Tokemak Network Audit

Staking Manual Review Findings

Staking Manual Review Findings

STA-01M: Improper Slashing Error Handling

TypeSeverityLocation
Logical FaultMinorStaking.sol:L369

Description:

The slash mechanism fatally fails if the amount to be slashed exceeds the availableToSlash amount which can change between a transaction's submission and a transaction's execution in the network.

Example:

contracts/staking/Staking.sol
344function slash(
345    address[] calldata accounts,
346    uint256[] calldata amounts,
347    uint256 scheduleIndex
348) external override onlyOwner whenNotPaused {
349    require(accounts.length == amounts.length, "LENGTH_MISMATCH");
350    StakingSchedule storage schedule = schedules[scheduleIndex];
351    require(schedule.setup, "INVALID_SCHEDULE");
352

353    for (uint256 i = 0; i < accounts.length; i++) {
354        address account = accounts[i];
355        uint256 amount = amounts[i];
356

357        require(amount > 0, "INVALID_AMOUNT");
358        require(account != address(0), "INVALID_ADDRESS");
359

360        StakingDetails memory userStake = userStakings[account][scheduleIndex];
361        require(userStake.initial > 0, "NO_VESTING");
362

363        uint256 availableToSlash = 0;
364        uint256 remaining = userStake.initial.sub(userStake.withdrawn);
365        if (remaining > userStake.slashed) {
366            availableToSlash = remaining.sub(userStake.slashed);
367        }
368

369        require(availableToSlash >= amount, "INSUFFICIENT_AVAILABLE");
370

371        userStake.slashed = userStake.slashed.add(amount);
372        userStakings[account][scheduleIndex] = userStake;
373

374        uint256 totalLeft = userStake.initial.sub((userStake.slashed.add(userStake.withdrawn)));
375

376        if (withdrawalRequestsByIndex[account][scheduleIndex].amount > totalLeft) {
377            withdrawalRequestsByIndex[account][scheduleIndex].amount = totalLeft;
378        }
379

380        uint256 voteAmount = totalLeft.sub(
381            withdrawalRequestsByIndex[account][scheduleIndex].amount
382        );
383        bytes32 eventSig = "Slashed";
384

385        encodeAndSendData(eventSig, account, scheduleIndex, voteAmount);
386

387        tokeToken.safeTransfer(treasury, amount);
388

389        emit Slashed(account, amount, scheduleIndex);
390    }
391}

Recommendation:

We advise better case handling to be introduced here whereby instead of using a require statement, the value of amount is set to availableToSlash if it exceeds it.

Alleviation:

The Tokemak team stated that the function should indeed fatally fail in case the amount slashed mismatches the on-chain balance given that this can also mean the off-chain calculations were performed incorrectly. As a result, we consider this exhibit null.

STA-02M: Unsanitized State Transition

TypeSeverityLocation
Logical FaultMinorStaking.sol:L537-L541

Description:

The setEventSend function should only set the _eventSend value to true when the values of the destinations struct have been set.

Example:

contracts/staking/Staking.sol
537function setEventSend(bool _eventSendSet) external override onlyOwner {
538    _eventSend = _eventSendSet;
539

540    emit EventSendSet(_eventSendSet);
541}

Recommendation:

We advise such sanitization to be imposed via corresponding require checks as otherwise almost all functions will become inexecutable.

Alleviation:

The function can now only be executed when the destinations.destinationOnL2 value has been set.

STA-03M: Potentially Incorrect Initialization

TypeSeverityLocation
Logical FaultInformationalStaking.sol:L78-L110

Description:

The Staking contract implementation is meant to act as an upgrade to a previously deployed implementation yet its initialize code appears to not factor this into account.

Example:

contracts/staking/Staking.sol
78function initialize(
79    IERC20 _tokeToken,
80    IManager _manager,
81    address _treasury,
82    address _scheduleZeroNotional
83) public initializer {
84    __Context_init_unchained();
85    __Ownable_init_unchained();
86    __Pausable_init_unchained();
87

88    require(address(_tokeToken) != address(0), "INVALID_TOKETOKEN");
89    require(address(_manager) != address(0), "INVALID_MANAGER");
90    require(_treasury != address(0), "INVALID_TREASURY");
91

92    tokeToken = _tokeToken;
93    manager = _manager;
94    treasury = _treasury;
95

96    //We want to be sure the schedule used for LP staking is first
97    //because the order in which withdraws happen need to start with LP stakes
98    _addSchedule(
99        StakingSchedule({
100            cliff: 0,
101            duration: 1,
102            interval: 1,
103            setup: true,
104            isActive: true,
105            hardStart: 0,
106            isPublic: true
107        }),
108        _scheduleZeroNotional
109    );
110}

Recommendation:

We advise the zero-based schedule addition to be re-evaluated as should this logic contract be deployed for an existing deployment, it will not properly initialize the zero-based staking schedule.

Alleviation:

The Tokemak team stated that the code reflects correct behaviour that replaces the previous implementation and ensures a correct state in the new logic upgrade.

Prev

Pool.sol (POO-M)

Next

SushiSwapControllerV2.sol (SSC-M)

On this page