hansfriese - TEL coins can be "locked" in plugins

[sherlock-admin]

hansfriese

high

TEL coins can be "locked" in plugins

Summary

Yields accrued in the plugins can be "locked" if the user exits without claiming yields.

Vulnerability Detail

StakingModule has an external function exit and this function withdraws the stake without claiming yields. If a user had some yields accrued in the plugin, it is very difficult to withdraw the unclaimed yields.

1. rescueTokens function in SimplePlugin.sol #158 can not withdraw the unclaimed yield because _totalOwed is not updated when the user calls exit.

/// @notice rescues any stuck erc20
/// @dev if the token is TEL, then it only allows maximum of balanceOf(this) - _totalOwed to be rescued
function rescueTokens(IERC20 token, address to) external onlyOwner {
  if (token == tel) {
    // if the token is TEL, only send the extra amount. Do not send anything that is meant for users.
    token.safeTransfer(to, token.balanceOf(address(this)) - _totalOwed);
  } else {
    // if the token isn't TEL, it's not supposed to be here. Send all of it.
    token.safeTransfer(to, token.balanceOf(address(this)));
  }
}

2. There is a recovery function claimAndExitFor in the StakingModule.sol #457 but it will take time and effort to find an account that exited without claiming yields from the events. Furthermore this function is callable only when the protocol is paused by an address with RECOVERY_ROLE.

function claimAndExitFor(
  address account,
  address to,
  bytes calldata auxData
) external onlyRole(RECOVERY_ROLE) whenPaused nonReentrant returns (uint256, uint256) {
  return (_claim(account, to, auxData), _exit(account, to));
}

From 2), it is difficult to say the funds are locked technically but I believe this is not what the protocol team intended. If deployed as it is, it is likely to be found out after a long time and the team will need to go through all events and find the accounts that caused inconsistency and call claimAndExitFor for each account one by one.

As a side note, the increasers might call increaseClaimableBy regardless of the user's staking status and I think there should be a way to retrieve the orphan yields from plugins. (The protocol can collect them or send to the user)

Impact

TEL coins can be stuck in plugins and it is even possible that they are forgotten.

Code Snippet

https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L457 https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/SimplePlugin.sol#L158 https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L167

Tool used

Manual Review

Recommendation

1. I don't know exactly why the team exposed a function exit without claiming yields but I recommend forcing claiming yields when the user exits. As far as I know, it is a common behavior for staking protocols to clean yields on exit.
2. I recommend adding a new mechanism to retrieve orphan yields from the plugins because increaseClaimableBy does not check the user's staking status at all, which means a user might be not aware of that they have claimable yields in the protocol.

[amshirif]

The user can still claim at a later date. Users are not required to claim at the time of unstaking, as collecting their reward counts as a taxable event. This allows the user to determine if they chose to take this on at the time. In addition, the rewards value passed into the FeeBuyback contract is calculated off chain. Here is where staking status is checked. All referral information is store off chain, so the FeeBuyback contract has no ability to determine the value of stake because the stake is not that of the use, but that of the referring party.