Incomplete support for MATIC token

Summary

While the current implementation deliberately handle the case of MATIC token, the support is still incomplete.

Vulnerability Detail

When the fee token is MATIC (0x0000000000000000000000000000000000001010), the secondary swap from fee token to TEL token will most certainly fail.

As it won't be able to pull funds from the safe, plus, 1inch _aggregator also can not pull funds from the feeBuyBack contract.

Impact

Whenever MATIC is used as the fee token, the transaction will fail.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Skip the secondary swap when the fee token is MATIC:

    //MATIC does not allow for approvals
    //ERC20s only
-    if (token != MATIC) {
+    if (token == MATIC) return false;
      IERC20(token).transferFrom(_safe, address(this), amount);
      IERC20(token).approve(_aggregator, amount);
-    }

amshirif commented 1 year ago

To handle MATIC, the currency is not pulled form the safe. (bool swapResult,) = _aggregator.call{value: msg.value}(swapData); Documentation should have been more complete. MATIC is passed in from the calling EOA and passed through to the aggregator at the same value the call is made with.

jack-the-pug commented 1 year ago

To handle MATIC, the currency is not pulled form the safe. (bool swapResult,) = _aggregator.call{value: msg.value}(swapData); Documentation should have been more complete. MATIC is passed in from the calling EOA and passed through to the aggregator at the same value the call is made with.

That's the native token MATIC, not this ERC20-like MATIC token: 0x0000000000000000000000000000000000001010, for this ERC20-like MACTI token (0x0000000000000000000000000000000000001010) the current system won't be able to support it.

amshirif commented 1 year ago

@jack-the-pug I think there might be some confusion. The Native MATIC and MRC20 Token (0x0000000000000000000000000000000000001010) are one in the same. To allow for Polygon's bridging system over from Ethereum, the MATIC contract was deployed in the Polygon Genesis block. I can provide some example transactions. Here is an address 0x8a2760e9ff6c76949119b94b22b2bfc64c91c44a. It only possesses Native MATIC. https://polygonscan.com/tx/0x3e34700bfd03f32b7d04968c61f5a1f1243d1cabf07a1db1c2ffb2f1fb9de6c2 https://polygonscan.com/tx/0x236d1f3fe581d3286012d8e2b01019f1f6b35372c0ad1a5578c015a4ba755fa4 Both transactions are 0.01 MATIC transfers to themselves. One is a simple transfer with no contract interaction, the other is a transfer interacting with the contract. If you look at the balance here https://polygonscan.com/address/0x8a2760e9ff6c76949119b94b22b2bfc64c91c44a, polygonscan will show an "ERC20" version once the address has interacted with the contract, but as you can see the balances are the same. If you take any address and do the MATIC.balanceOf() call on the MATIC contract it will always return the same balance as web3.eth.getBalance()

Evert0x commented 1 year ago

We have reason to believe that this erc20 and gas is treated the same by polygon on accounting level, but think there is still a difference on the EVM level.

Either way, we are not completely sure and wait for @jack-the-pug his answer.

Closing the issue for now to announce the preliminary results.