ThrottledExit Manual Review Findings

TET-01M: Code Bottleneck

Description:

The linked function, getAvailableExitTime, requires refactoring to be able to be considered production ready as at its current state is ill constructed. The inner while loop can lead to a DoS in case of prolonged inactivity whilst the general way the logical paths of the code block are executed is inefficient and can become prohibitively expensive so.

Example:

Copy
68function getAvailableExitTime(uint256 exitAmount) internal returns(uint256 exitBlock) {
69    if(block.number > nextAvailableExitBlock) { // We've passed the next available block and need to readjust
70        uint i = nextAvailableExitBlock; // Using i instead of nextAvailableExitBlock to avoid SSTORE
71        while(i < block.number) { // Find the next future round
72            i = i.add(throttleRoundBlocks);
73        }
74        nextAvailableExitBlock = i;
75        nextAvailableRoundExitVolume = exitAmount; // Reset volume
76        return i;
77    } else { // We are still before the next available block
78        nextAvailableRoundExitVolume = nextAvailableRoundExitVolume.add(exitAmount); // Add volume
79    }
80
81    exitBlock = nextAvailableExitBlock;
82
83    if (nextAvailableRoundExitVolume >= throttleRoundCap) { // If cap reached
84        nextAvailableExitBlock = nextAvailableExitBlock.add(throttleRoundBlocks); // update next exit block
85        nextAvailableRoundExitVolume = 0; // Reset volume
86    }
87}

Recommendation:

We highly advise the code block to be refactored as well as renamed to properly fall in line with the quality of the other segments of the codebase and increase in legibility.

Alleviation:

Although the function was optimized by applying our exhibit, we still believe it to be a pain point of the system. Our suggestion to rename the function properly reflecting its functionality remains.

TET-02M: Inapplicacy of Checks-Effects-Interactions Pattern

Description:

The linked statements conduct external calls with values retrieved from storage that are zeroed out after they have been utilized by the external call.

Example:

Copy
53function finalizeExit(address _stakingToken, address[] memory _rewardsTokens) virtual internal {
54    ExitInfo storage info = exitInfo[msg.sender];
55    require(block.number > info.exitBlock, "finalizeExit::Trying to exit too early");
56
57    IERC20Detailed(_stakingToken).safeTransfer(address(msg.sender), info.exitStake);
58
59    for (uint256 i = 0; i < _rewardsTokens.length; i++) {
60        IERC20Detailed(_rewardsTokens[i]).safeTransfer(msg.sender, info.rewards[i]);
61        info.rewards[i] = 0;
62       }
63
64    emit ExitCompleted(msg.sender, info.exitStake);
65    info.exitStake = 0;
66}

Recommendation:

As external calls can lead to a re-entrancy, it is always best practice to apply the Checks-Effects-Interactions pattern. In the linked cases, the code should store the variables to a temporary in-memory slot, zero them out and then perform the external call they are meant to accompany. The severity of this finding has been labelled as "minor" due to the _rewardTokens being defined by the owner of the perspective factories, however, the pattern should be applied regardless to ensure security by design.

Alleviation:

The Checks-Effects-Interactions pattern has been applied to the codebase by first zeroing out any arguments that are consequently passed to the external interactions.

TET-03M: Incorrect Cap Management

Description:

The linked if clause is meant to cover the case whereby the round exit volume has exceeded the throttleRoundCap and progress the nextAvailableExitBlock by throttleRoundBlocks before zeroing the cap.

Example:

Copy
83if (nextAvailableRoundExitVolume >= throttleRoundCap) { // If cap reached
84    nextAvailableExitBlock = nextAvailableExitBlock.add(throttleRoundBlocks); // update next exit block
85    nextAvailableRoundExitVolume = 0; // Reset volume
86}

Recommendation:

The current implementation does not factor in the case of the exitAmount being equal to or greater than the throttleRoundCap thus requiring that the nextAvailableExitBlock is offset by twice the throttle rounds. We advise that the calculation at L84 takes this into account by multiplying throttleRoundBlocks by nextAvailableRoundExitVolume divided by throttleRoundCap yielding the correct number of throttle rounds that should be added.

Alleviation:

Although comments were added, they are slightly misleading in the sense that they refer to the last user going "a little bit" over the cap. It is possible to arbitrarily exceed the cap even by two-three rounds worth of volume.

TET-04M: Multi-Transaction Reversal

Description:

The safeTransfer statement linked by the exhibit is conducted on all members of the _rewardTokens array sequentially, yet if a single token is unable to perform the transfer the exit fails and all reward token payouts revert.

Example:

Copy
53function finalizeExit(address _stakingToken, address[] memory _rewardsTokens) virtual internal {
54    ExitInfo storage info = exitInfo[msg.sender];
55    require(block.number > info.exitBlock, "finalizeExit::Trying to exit too early");
56
57    IERC20Detailed(_stakingToken).safeTransfer(address(msg.sender), info.exitStake);
58
59    for (uint256 i = 0; i < _rewardsTokens.length; i++) {
60        IERC20Detailed(_rewardsTokens[i]).safeTransfer(msg.sender, info.rewards[i]);
61        info.rewards[i] = 0;
62       }
63
64    emit ExitCompleted(msg.sender, info.exitStake);
65    info.exitStake = 0;
66}

Recommendation:

As finalizing an exit is a crucial system component, we advise that the reward tokens are instead optimistically transferred rather than required to all succeed in their transfers. This would prevent a scenario whereby a single reward token failing causes all exits to be permanently halted.

Alleviation:

The codebase remains unaltered, indicating that it indeed is the intention for all transfers to revert.