Deposit Box Does Not Account for Fee On Transfer Tokens Causing the Bridge To Absorb the Fees

[code423n4]

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L95-L132

Vulnerability details

Impact

The function DepositBoxERC20.depositERC20() does not account for FoT (Fee on Transfer) tokens. FoT tokens charge a fee when transfer() or transferFrom() is called and it is subtracted from amount so the receiving address will receive less than amount of tokens.

This occurs in depositERC20() since the original amount is transferred through the bridge to the ERC20 token on the SChain. However, this amount may have fees deducted from it during the following

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

The impact is that the bridge will end up absorbing all of the fees for deposits.

Proof of Concept

    function depositERC20(
        string calldata schainName,
        address erc20OnMainnet,
        uint256 amount
    )
        external
        override
        rightTransaction(schainName, msg.sender)
        whenNotKilled(keccak256(abi.encodePacked(schainName)))
    {
        bytes32 schainHash = keccak256(abi.encodePacked(schainName));
        address contractReceiver = schainLinks[schainHash];
        require(contractReceiver != address(0), "Unconnected chain");
        require(
            ERC20Upgradeable(erc20OnMainnet).allowance(msg.sender, address(this)) >= amount,
            "DepositBox was not approved for ERC20 token"
        );
        bytes memory data = _receiveERC20(
            schainName,
            erc20OnMainnet,
            msg.sender,
            amount
        );
        _saveTransferredAmount(schainHash, erc20OnMainnet, amount);
        require(
            ERC20Upgradeable(erc20OnMainnet).transferFrom(
                msg.sender,
                address(this),
                amount
            ),
            "Transfer was failed"
        );
        messageProxy.postOutgoingMessage(
            schainHash,
            contractReceiver,
            data
        );
    }

Recommended Mitigation Steps

Consider preventing fee on transfer tokens from being used in the system. This must be done either by only allowing whitelisted addresses.

Alternatively, this can be done by ensuring the balance of the Deposit Box increases by exactly amount. For example.

        uint256 balanceBefore = ERC20Upgradeable(erc20OnMainnet).balanceOf(address(this));
        require(
            ERC20Upgradeable(erc20OnMainnet).transferFrom(
                msg.sender,
                address(this),
                amount
            ),
        require(ERC20Upgradeable(erc20OnMainnet).balanceOf(address(this)) == balanceBefore + amount)

[cstrangedk]

Reference #8, #9, #10, #12, #19

Only standard functions are applied to out-of-the-box DepositBoxes and TokenManagers. It's up to the SKALE chain owner's discretion to create custom DepositBoxes/TokenManagers that specifically support non-standard functions like FoT, and add these custom contracts to the bridge using registerExtraContract() functions in MessageProxy.

[GalloDaSballo]

Dup of #50