hyh - FeeBuyback's submit can lose funds if used with zero addresses, which is allowed

[sherlock-admin]

hyh

medium

FeeBuyback's submit can lose funds if used with zero addresses, which is allowed

Summary

It's now allowed to use FeeBuyback's submit() with zero wallet or zero _aggregator, while low level calls are performed with both addresses.

Vulnerability Detail

Low-level calls submit() perform will return success if used with zero addresses as the failure need to come from the contract that is being called, while zero address will not do that. In both cases no operations will be performed.

Impact

Not executing the primary swap, but paying the fee is the fund loss for an owner.

Executing the primary swap, but not paying the fee as there is no TEL on the balance is the fund loss for a recipient.

Also, when MATIC is used the native funds sent to zero address via _aggregator.call{value: msg.value}(swapData) end up being lost.

Code Snippet

Both _aggregator (in constructor) and wallet (in submit() below) aren't checked to be non-zero:

https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/fee-buyback/FeeBuyback.sol#L28-L33

  constructor(address aggregator_, address safe_, IERC20 telcoin_, ISimplePlugin referral_) TieredOwnership() {
    _aggregator = aggregator_;
    _safe = safe_;
    _telcoin = telcoin_;
    _referral = referral_;
  }

Low-level calls submit() perform will be successful if the address called (wallet, or _aggregator later) is zero:

https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/fee-buyback/FeeBuyback.sol#L35-L82

  /**
   * @notice submits wallet transactions
   * @dev a secondary swap may occur
   * @dev staking contract updates may be made
   * @dev function can be paused
   * @param wallet address of the primary transaction
   * @param walletData bytes wallet data for primary transaction
   * @param token address the token that is being swapped from in a secondary transaction
   * @param amount uint256 the quantity of the token being swapped
   * @param swapData bytes swap data from primary transaction
   * @return boolean representing if a referral transaction was made
   */
  function submit(address wallet, bytes memory walletData, address token, address recipient, uint256 amount, bytes memory swapData) external override payable onlyOwner() returns (bool) {
    //Perform user swap first
    //Verify success
    (bool walletResult,) = wallet.call{value: 0}(walletData);
    require(walletResult, "FeeBuyback: wallet transaction failed");

    ...

    //Perform secondary swap from fee token to TEL
    //do simple transfer from and submit
    (bool swapResult,) = _aggregator.call{value: msg.value}(swapData);
    require(swapResult, "FeeBuyback: swap transaction failed");
    _telcoin.approve(address(_referral), _telcoin.balanceOf(address(this)));
    require(_referral.increaseClaimableBy(recipient, _telcoin.balanceOf(address(this))), "FeeBuyback: balance was not adjusted");
    return true;
  }

Notice, that _telcoin.balanceOf(address(this)) is allowed to be zero, which can happen as the secondary swap become a noop when _aggregator is zero.

In this case the funds will be lost as the native funds transfer executes even if the address is zero, while for the reverting to happen the contract have to indicate this, which will not be happening.

Tool used

Manual Review

Recommendation

Control for both _aggregator and wallet to be non-zero:

https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/fee-buyback/FeeBuyback.sol#L28-L33

  constructor(address aggregator_, address safe_, IERC20 telcoin_, ISimplePlugin referral_) TieredOwnership() {
+   require(_aggregator != address(0), "FeeBuyback: zero aggregator");
    _aggregator = aggregator_;
    _safe = safe_;
    _telcoin = telcoin_;
    _referral = referral_;
  }

https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/fee-buyback/FeeBuyback.sol#L35-L82

  /**
   * @notice submits wallet transactions
   * @dev a secondary swap may occur
   * @dev staking contract updates may be made
   * @dev function can be paused
   * @param wallet address of the primary transaction
   * @param walletData bytes wallet data for primary transaction
   * @param token address the token that is being swapped from in a secondary transaction
   * @param amount uint256 the quantity of the token being swapped
   * @param swapData bytes swap data from primary transaction
   * @return boolean representing if a referral transaction was made
   */
  function submit(address wallet, bytes memory walletData, address token, address recipient, uint256 amount, bytes memory swapData) external override payable onlyOwner() returns (bool) {
+   require(wallet != address(0), "FeeBuyback: zero wallet");
    //Perform user swap first
    //Verify success
    (bool walletResult,) = wallet.call{value: 0}(walletData);
    ...

[amshirif]

https://github.com/telcoin/telcoin-staking/pull/7

[hrishibhat]

Zero address check in the constructor. Low/Informational.

[dmitriia]

Escalate for 10 USDC submit() is a routine operation, and contrary to configuration setting level checks, have higher probability of the operational mistake. I.e. despite being onlyOwner it will be run daily/weekly, not as a once a year setup where the checks will be just moved to deploy script. The impact here is irreversible loss of principal funds. Despite being input check suggestion Med is viable for the wallet part. _aggregator check in constructor is indeed low, was included here for completeness, having similar root cause.

[sherlock-admin]

Escalate for 10 USDC submit() is a routine operation, and contrary to configuration setting level checks, have higher probability of the operational mistake. I.e. despite being onlyOwner it will be run daily/weekly, not as a once a year setup where the checks will be just moved to deploy script. The impact here is irreversible loss of principal funds. Despite being input check suggestion Med is viable for the wallet part. _aggregator check in constructor is indeed low, was included here for completeness, having similar root cause.

You've created a valid escalation for 10 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

Zero address check, anticipating an operational mistake from a trusted admin is not a valid medium.

[sherlock-admin]

Escalation rejected

Zero address check, anticipating an operational mistake from a trusted admin is not a valid medium.

This issue's escalations have been rejected!

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