hats-finance / Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d

Aleph Zero bridge to Ethereum
Apache License 2.0
0 stars 1 forks source link

Fee-on-Transfer Tokens Could Cause Different Balances Between Ethereum and Azero Side #43

Open hats-bug-reporter[bot] opened 5 months ago

hats-bug-reporter[bot] commented 5 months ago

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xd23ef70da434a0a366e7eb0245018d0b5ab43fc0235cf976c13a1ca2dbc23418 Severity: medium

Description: Description\ The sendRequest function transfers tokens from msg.sender. However, some tokens implement fee-on-transfer mechanisms, which means they don't transfer the actual amount specified. This inconsistency can cause sendRequestNative transactions failed on transfering tokens later on.

Impact\ This discrepancy in token balances between the Ethereum and Azero chains can lead to transaction failures in the receiveRequest function. While this issue is recoverable by the owner.

Scenario\ User A locks some tokens with a fee-on-transfer mechanism in the contract, initially transferring 100 tokens. However, due to the fee-on-transfer mechanism, only 99.98 tokens are actually transferred. Later, when the user attempts to unlock the tokens, the sendRequestNative function failed ot transfer tokens because the expected balance of 100 tokens is not present.

Poc\

pragma solidity ^0.8.20;

import "forge-std/Test.sol"; import "../contracts/Most.sol"; import "forge-std/console.sol";

interface IPAXG { function transfer(address _to, uint256 _value) external returns (bool);

function approve(address _spender, uint256 _value) external returns (bool);

function balanceOf(address _addr) external view returns (uint256);

}

contract POC is Test { event TokenTransferFailed(bytes32 requestHash);

Most public most;
IPAXG public paxg;

function setUp() public {
    most = new Most();
    paxg = IPAXG(0x45804880De22913dAFE09f4980848ECE6EcbAf78);
}

address alice = makeAddr("alice");
address public WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;

function test_poc() public {

    //setup
    address[] memory addresses = new address[](1);
    addresses[0] = alice;
    most.initialize(addresses, 1, alice, payable(WETH));
    uint256 amount = 100e18;
    deal(address(paxg), alice, amount);
    vm.startPrank(alice);
    most.addPair(bytes32(uint256(uint160(address(paxg)))), bytes32(uint256(uint160(address(paxg)))));
    most.unpause();

    // send request 100e18 (amount)
    console.log(amount, "amount send request");
    paxg.approve(address(most), amount);
    most.sendRequest(bytes32(uint256(uint160(address(paxg)))), amount, bytes32(uint256(uint160(alice))));
    console.log(paxg.balanceOf(address(most)), "most balance = actual amount");

    uint256 _committeeId = 0;
    bytes32 destTokenAddress = bytes32(uint256(uint160(address(paxg))));
    bytes32 destReceiverAddress = bytes32(uint256(uint160(alice)));
    uint256 _requestNonce = 0;
    bytes32 requestHash =
        keccak256(abi.encodePacked(_committeeId, destTokenAddress, amount, destReceiverAddress, _requestNonce));

    // transfer fail as balance of most is less than actual amount (100e18)
    vm.expectEmit();
    emit TokenTransferFailed(requestHash);
    most.receiveRequest(requestHash, _committeeId, destTokenAddress, amount, destReceiverAddress, _requestNonce);

}

}


output:

Running 1 test for test/poc3.t.sol:POC [PASS] test_poc() (gas: 546406) Logs: 100000000000000000000 amount send request 99980000000000000000 most balance = actual amount

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 20.67ms

**Revised Code File (Optional)**\
Calculate the transferred amount based on the balance before and after the transfer.

```diff
         bytes32 destTokenAddress = supportedPairs[srcTokenAddress];
         if (destTokenAddress == 0x0) revert UnsupportedPair();
+        uint256 balanceBefore = token.balanceOf(address(this));

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

-        emit CrosschainTransferRequest(committeeId, destTokenAddress, amount, destReceiverAddress, requestNonce);
+        emit CrosschainTransferRequest(
+            committeeId, destTokenAddress, balanceAfter - balanceBefore, destReceiverAddress, requestNonce
+        );

         ++requestNonce;
     }
krzysztofziobro commented 4 months ago

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.