code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L120-L124 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L306-L308 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L206

Vulnerability details

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to ERC20Upgradeable, their function signatures do not match and therefore the calls made will revert.

Impact

The code as currently implemented does not handle these sorts of tokens properly when they're deposited or withdrawn. Tether cannot be deposited because depositing requires a cast and a call to transferFrom() which will revert. If a token has a different signature for transfer() but the correct one for transferFrom() the user's tokens will be stuck.

Proof of Concept

            ERC20Upgradeable(erc20OnMainnet).transferFrom(
                msg.sender,
                address(this),
                amount
            ),

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L120-L124

    function _removeTransferredAmount(bytes32 schainHash, address erc20Token, uint256 amount) private {
        transferredAmount[schainHash][erc20Token] -= amount;
    }

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L306-L308

            ERC20Upgradeable(erc20OnMainnet).transfer(receiver, amount),

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L206

Tools Used

Code inspection

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20Upgradeable's safeTransfer() and safeTransferFrom() instead

cstrangedk commented 2 years ago

Reference/duplicate disputed #8, #9, #10, #12, #19.

Disputed as SKALE Chain owner can customize new depositbox contract to use safetransfer functions if needed, and use registerExtraContract().

GalloDaSballo commented 2 years ago

Dup of #8

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