hansfriese - Plugin need to be checked on addition

[sherlock-admin]

hansfriese

medium

Plugin need to be checked on addition

Summary

The plugin should be checked if it implemented the IPlugin when it is newly added. NOTE: This report contains a few other low-level issues as well and the sponsor encouraged sending them grouped as a Med level.

Vulnerability Detail

1. The function addPlugin at StakingModule.sol#L409 does not check if the new plugin implemented the IPlugin interface. Because major functions iterates all plugins for various purposes, a wrong plugin will make the whole protocol broken. Although this function is restricted to PLUGIN_EDITOR_ROLE only and it is possible to fix by removing the wrong plugin, I rate this as a medium level vulnerability because it has a critical effect and the role is not a strict admin role.

2. Some other low-level issues

• rescueERC20 function at FeeBuyback.sol#L94 always return true while it is supposed to return true only when the transfer succeeds from the comments.

/**
 * @notice Sends ERC20 tokens trapped in contract to external address
 * @dev Only an owner is allowed to make this function call
 * @param account is the receiving address
 * @param externalToken is the token being sent
 * @param amount is the quantity being sent
 * @return boolean value indicating whether the operation succeeded.
 *
 * Emits a {Transfer} event.
 */
function rescueERC20(address account, address externalToken, uint256 amount) public onlyExecutor returns (bool) {
  IERC20(externalToken).transfer(account, amount);
  return true; //@audit different from the comment
}

• At StakingModule.sol#L378, the protocol always emits StakeChanged events while it is supposed to be emitted only when oldStake != newStake.
• At StakingModule.sol#L60, the function initialize is declared as payable and I don't see any reasons for that.
• At SimplePlugin.sol#L150, the function setIncreaser does not check if the newIncreaser is different from the current increaser and an unnecessary event will be emitted.

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#L409 https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/fee-buyback/FeeBuyback.sol#L94 https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L378 https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L60 https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/SimplePlugin.sol#L150

Tool used

Manual Review

Recommendation

1. Check the validity of the new plugin in the function addPlugin.
2. Check the transfer result and return it in the function rescueERC20 of FeeBuyback.sol.
3. Check if oldStake!=newStake at StakingModule.sol#L378.
4. Remove payable keyword from the StakingModule initializer.
5. Check if the newIncreaser is different from the current increaser at SimplePlugin.sol#L150.

[amshirif]

https://github.com/telcoin/telcoin-staking/pull/6 Some changes regarding rescueERC made in separate pull

[hrishibhat]

Input validation suggestion for a function with trusted role access. Considering as low.

[ctf-sec]

Escalate for 1 USDC.

The function below is also in the control of onlyOwner so I think adding validating msg.value is equal to amount is also a input validation suggestion. If issue 76 is valid and eligible for a reward, then this issue should be eligible for the reward as well.

  function submit(address wallet, bytes memory walletData, address token, address recipient, uint256 amount, bytes memory swapData) external override payable onlyOwner() returns (bool) {

https://github.com/sherlock-audit/2022-11-telcoin-judging/issues/76

Also, Sherlock has been rewarding the same finding here:

https://github.com/sherlock-audit/2022-10-mycelium-judging/tree/main/010-M

So I think this issue should be considered as a valid one.

Thanks.

[sherlock-admin]

Escalate for 1 USDC.

The function below is also in the control of onlyOwner so I think adding validating msg.value is equal to amount is also a input validation suggestion. If issue 76 is valid and eligible for a reward, then this issue should be eligible for the reward as well.

  function submit(address wallet, bytes memory walletData, address token, address recipient, uint256 amount, bytes memory swapData) external override payable onlyOwner() returns (bool) {

https://github.com/sherlock-audit/2022-11-telcoin-judging/issues/76

Also, Sherlock has been rewarding the same finding here:

https://github.com/sherlock-audit/2022-10-mycelium-judging/tree/main/010-M

So I think this issue should be considered as a valid one.

Thanks.

You've created a valid escalation for 1 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[gguney]

Escalate for 5 USDC My https://github.com/sherlock-audit/2022-11-telcoin-judging/issues/49 issue was marked as duplicate of this issue. This issue is confirmed but mine was ignored.

[sherlock-admin]

Escalate for 5 USDC My https://github.com/sherlock-audit/2022-11-telcoin-judging/issues/49 issue was marked as duplicate of this issue. This issue is confirmed but mine was ignored.

You've created a valid escalation for 5 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation rejected

So addplugin in mycelium is medium severity as there is a risk of irreversible damage because of external function calls on the contract. In this case, there are no external calls so plugins can always be removed.

[sherlock-admin]

Escalation rejected

So addplugin in mycelium is medium severity as there is a risk of irreversible damage because of external function calls on the contract. In this case, there are no external calls so plugins can always be removed.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.