Platform: Code4rena
Start Date: 18/02/2022
Pot Size: $125,000 USDC
Total HM: 13
Participants: 24
Period: 14 days
Judge: GalloDaSballo
Total Solo HM: 6
Id: 88
League: ETH
Rank: 12/24
Findings: 2
Award: $2,008.75
🌟 Selected for report: 1
🚀 Solo Findings: 0
1618.2353 USDC - $1,618.24
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
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.
Manual code review
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.
#0 - cstrangedk
2022-03-03T19:56:26Z
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.
#1 - GalloDaSballo
2022-06-01T16:16:54Z
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
390.5096 USDC - $390.51
In TokenManagerERC721.sol the _sendERC721() function does not protect the user by making use of transferFrom. transferFrom on ERC-721 tokens does not check the recipient for whether they are capable of supporting the token transfer. This means that tokens can be stuck in the recipient contracts that weren't designed to handle them.
https://samczsun.com/the-dangers-of-surprising-code/
Manual code review
safeTransferFrom should be used with a reentrancy guard modifier on the _sendERC721() function since the caller will receive a callback. This will provide a safeGuard for the user and protect the protocol as well from reentrancy attacks.
#0 - cstrangedk
2022-03-03T20:59:53Z
Referencing #8
It is at the SKALE Chain owners discretion to use standard transfer...() functions in off-the-shelf contracts, or add custom contracts to support safeTransfer() by using registerExtraContract() functions.
#1 - GalloDaSballo
2022-06-01T16:22:40Z
I agree with the sponsor reasoning that the choice is for the Admin to configure the contracts.
That said these contracts are in scope and as such the swords cuts both ways in that the Admin could use these.
For that reason I believe we cannot have a High Severity (contingent on configuration) but findings can still be valid.
That said I have an issue with this one: if the transfer to the recipient were to be reverted due to the safeErc721Transfer this may cause further issues.
Given this shaky premise I believe forcing to use safeErc721Transfer could cause DOS and further issues.
I do believe the finding to have some validity however I think Low Severity to be more appropriate
#2 - JeeberC4
2022-06-02T16:26:05Z
Creating QA Report for warden as judge downgraded issue. Preserving original title: users not protected using transferFrom on ERC721 tokens