code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/schain/TokenManagers/TokenManagerERC20.sol#L298 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/schain/TokenManagers/TokenManagerERC20.sol#L303

Vulnerability details

Impact

In TokenManagerERC20.sol the _exit() function makes use of transferFrom() instead of using safeTransferFrom(). Tokens that don’t correctly implement the latest EIP20 spec will be unusable in the protocol as they revert the transaction because of the missing return value.

Proof of Concept

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/schain/TokenManagers/TokenManagerERC20.sol#L298

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/schain/TokenManagers/TokenManagerERC20.sol#L303

Tools Used

Manual code review

Recommended Mitigation Steps

It's recommended to use OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

cstrangedk commented 2 years ago

Issue is acknowledged and work is pending on the roadmap. No loss of funds is possible, only revert txn. In the meantime, SKALE Chain owners at their discretion can expand the bridge compatibility to use safeTransfer functions. Owners must evaluate token compatibility.

GalloDaSballo commented 2 years ago

I believe that given the "need for configuration" the finding cannot be of High Severity. Additionally, the tx will revert as such no loss of funds is possible.

The tokens that will cause a revert (e.g. USDT) will simply be unusable.

While the argument for configuration is correct in de-escalating to Medium, I don't believe it exempts the code from being properly scrutinized.

If a user were to configure their chain to use TokenManagerERC20 they'd have revert on non returning tokens, for that reason I believe Medium Severity to be appropriate as this is contingent on configuration

code-423n4 / 2022-02-skale-findings