Fee on transfer tokens are unsupported and will have accounting issues

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x0cc5d64fcb1309e8ea2a654336ebcb870e364cc55ea00ec7eed9ab765aec8c5b Severity: low

Description:

Impact

Unexpected functionality, possible loss of funds up to the expected fees.

Desription

The current implementation does not support fee on transfer token bridging. Currently if a user intends to bridge an ERC-20 with fees, either the relayer has to overpay for it or the value on the AlephZero side will be inflated or the transfer will fail.

Most.sol - sendRequest()

    function sendRequest(
        bytes32 srcTokenAddress,
        uint256 amount,
        bytes32 destReceiverAddress
    ) external whenNotPaused {
        if (amount == 0) revert ZeroAmount();

        IERC20 token = IERC20(bytes32ToAddress(srcTokenAddress));

        bytes32 destTokenAddress = supportedPairs[srcTokenAddress];
        if (destTokenAddress == 0x0) revert UnsupportedPair();

        // lock tokens in this contract
        // message sender needs to give approval else this tx will revert
        token.safeTransferFrom(msg.sender, address(this), amount);

        emit CrosschainTransferRequest( 
            committeeId,
            destTokenAddress,
            amount,
            destReceiverAddress,
            requestNonce
        );

        ++requestNonce;
    }

Since tokens have to be manually added by the governance via addPair() I decided on low severity.

Recommendation

Consider whether it's worth to add support for fee-on-transfer tokens, otherwise document that such tokens are not supported by the bridge. I will provide an example on how to calculate the balance correctly with fee tokens:

uint256 balanceBefore = IERC20(token).balanceOf(address(this));
IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
uint256 balanceAfter = IERC20(token).balanceOf(address(this));
uint256 sendAmount = balanceAfter - balanceBefore;

token.safeTransferFrom(msg.sender, address(this), sendAmount);

[0xfuje]

• rebasing tokens are uncompatible as well

[krzysztofziobro]

The protocol is not intended to support any tokens with non-standard indirect balance changes. Whitelisting such a token would be considered an owner error.