imp(contracts/ics20): add more balance validation to permit2 transfers (backport #954) (#987)

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>
Co-authored-by: srdtrk <srdtrk@hotmail.com>

Author: mergify[bot]

.github/actions/e2e-setup/action.yml

@@ -18,43 +18,28 @@ runs:
         check-latest: true
         cache-dependency-path: e2e/interchaintestv8/go.sum
 
-    - name: Cache Relayer and Operator
-      id: cache-relayer
-      uses: actions/cache@v4
-      with:
-        path: |
-          ~/.cargo/bin/relayer
-          ~/.cargo/bin/operator
-          ./target/elf-compilation/riscv32im-succinct-zkvm-elf/release/*
-        key: ${{ runner.os }}-relayer-${{ hashFiles('Cargo.lock', 'Cargo.toml', 'packages/**', 'programs/**', 'abi/**') }}
-
     - name: Install SP1 toolchain
-      if: (steps.cache-relayer.outputs.cache-hit != 'true')
       shell: bash
       run: |
         curl -L https://sp1.succinct.xyz | bash
-        ~/.sp1/bin/sp1up --token ${{ inputs.github_token }}
+        ~/.sp1/bin/sp1up --token ${{ inputs.github_token }} -v v5.2.4
         ~/.sp1/bin/cargo-prove prove --version
 
     - name: Install operator
-      if: steps.cache-relayer.outputs.cache-hit != 'true'
       uses: actions-rs/cargo@v1
       with:
         command: install
         args: --bin operator --path programs/operator --locked
 
     - name: Install relayer
-      if: steps.cache-relayer.outputs.cache-hit != 'true'
       uses: actions-rs/cargo@v1
       with:
         command: install
         args: --bin relayer --path programs/relayer --locked
 
     - name: Install just
-      if: (steps.cache-relayer.outputs.cache-hit != 'true')
       uses: extractions/setup-just@v2
     - name: Build SP1 Programs
-      if: (steps.cache-relayer.outputs.cache-hit != 'true')
       shell: bash
       run: just build-sp1-programs
 

.github/actions/foundry-setup/action.yml

@@ -4,11 +4,13 @@ runs:
   using: composite
   steps:
     - name: Set up Rust
-      uses: dtolnay/rust-toolchain@stable
+      uses: dtolnay/rust-toolchain@1.87.0
       with:
         components: rustfmt, clippy
     - name: Install Foundry
       uses: foundry-rs/foundry-toolchain@v1
+      with:
+        version: v1.1.0
     - name: Install Bun
       uses: oven-sh/setup-bun@v2
     - name: Install the Node.js dependencies

.github/workflows/release.yml

@@ -11,7 +11,7 @@ jobs:
     steps:
       - uses: actions/checkout@v4
       - name: Set up Rust
-        uses: dtolnay/rust-toolchain@stable
+        uses: dtolnay/rust-toolchain@1.87.0
       - name: Install SP1 toolchain
         shell: bash
         run: |

.github/workflows/rust.yml

@@ -82,7 +82,7 @@ jobs:
     steps:
       - uses: actions/checkout@v4
       - name: Set up Rust
-        uses: dtolnay/rust-toolchain@stable
+        uses: dtolnay/rust-toolchain@1.87.0
         with:
           components: rustfmt, clippy
           targets: wasm32-unknown-unknown

contracts/ICS20Transfer.sol

@@ -177,12 +177,7 @@ contract ICS20Transfer is
         );
         // transfer the tokens to us with permit
         IEscrow escrow = _getOrCreateEscrow(msg_.sourceClient);
-        _getPermit2().permitTransferFrom(
-            permit,
-            ISignatureTransfer.SignatureTransferDetails({ to: address(escrow), requestedAmount: msg_.amount }),
-            _msgSender(),
-            signature
-        );
+        _transferFromWithPermit2(_msgSender(), address(escrow), msg_.denom, msg_.amount, permit, signature);
         escrow.recvCallback(msg_.denom, _msgSender(), msg_.amount);
 
         return _sendTransferFromEscrowWithSender(msg_, address(escrow), _msgSender());
@@ -435,6 +430,50 @@ contract ICS20Transfer is
         );
     }
 
+    /// @notice Transfer tokens from sender to receiver using permit2
+    /// @param sender The sender of the tokens
+    /// @param receiver The receiver of the tokens
+    /// @param tokenContract The address of the token contract
+    /// @param amount The amount of tokens to transfer
+    /// @param permit The permit data
+    /// @param signature The signature of the permit data
+    // NOTE: Balance reentrancy here is intentionally allowed, as we are checking the balance before and after
+    // slither-disable-next-line reentrancy-balance
+    function _transferFromWithPermit2(
+        address sender,
+        address receiver,
+        address tokenContract,
+        uint256 amount,
+        ISignatureTransfer.PermitTransferFrom calldata permit,
+        bytes calldata signature
+    )
+        private
+    {
+        // we snapshot current balance of this token
+        uint256 ourStartingBalance = IERC20(tokenContract).balanceOf(receiver);
+
+        _getPermit2().permitTransferFrom(
+            permit,
+            ISignatureTransfer.SignatureTransferDetails({ to: receiver, requestedAmount: amount }),
+            sender,
+            signature
+        );
+
+        // check what this particular ERC20 implementation actually gave us, since it doesn't
+        // have to be at all related to the _amount
+        uint256 actualEndingBalance = IERC20(tokenContract).balanceOf(receiver);
+
+        uint256 expectedEndingBalance = ourStartingBalance + amount;
+        // a very strange ERC20 may trigger this condition, if we didn't have this we would
+        // underflow, so it's mostly just an error message printer
+        // NOTE: This is not a security check, but rather a sanity check.
+        // slither-disable-next-line incorrect-equality
+        require(
+            actualEndingBalance > ourStartingBalance && actualEndingBalance == expectedEndingBalance,
+            ICS20UnexpectedERC20Balance(expectedEndingBalance, actualEndingBalance)
+        );
+    }
+
     /// @notice Finds a contract in the foreign mapping, or creates a new IBCERC20 contract
     /// @notice This function will never return address(0)
     /// @param fullDenomPath The full path denom to find or create the contract for (which will be the name for the

test/solidity-ibc/ICS20TransferTest.t.sol

@@ -15,7 +15,7 @@ import { IICS26Router } from "../../contracts/interfaces/IICS26Router.sol";
 import { IIBCUUPSUpgradeable } from "../../contracts/interfaces/IIBCUUPSUpgradeable.sol";
 
 import { ICS20Transfer } from "../../contracts/ICS20Transfer.sol";
-import { TestERC20, MalfunctioningERC20 } from "./mocks/TestERC20.sol";
+import { TestERC20, MalfunctioningERC20, FeeOnTransferERC20 } from "./mocks/TestERC20.sol";
 import { ICS20Lib } from "../../contracts/utils/ICS20Lib.sol";
 import { ICS24Host } from "../../contracts/utils/ICS24Host.sol";
 import { Strings } from "@openzeppelin-contracts/utils/Strings.sol";
@@ -196,9 +196,30 @@ contract ICS20TransferTest is Test, DeployPermit2, PermitSignature {
         vm.prank(sender);
         ics20Transfer.sendTransferWithPermit2(_getTestSendTransferMsg(), permit, invalidSignature);
 
-        // prove that it works with a valid signature
+        // ===== Case: ERC20 token with fee on transfer, where the balance after transfer is less than expected =====
+        IICS20TransferMsgs.SendTransferMsg memory msgSendTransfer = _getTestSendTransferMsg();
+
+        FeeOnTransferERC20 feeOnTransferERC20 = new FeeOnTransferERC20(); // 1 unit fee on every transfer
+
+        msgSendTransfer.denom = address(feeOnTransferERC20);
+
+        ISignatureTransfer.PermitTransferFrom memory feePermit = ISignatureTransfer.PermitTransferFrom({
+            permitted: ISignatureTransfer.TokenPermissions({ token: address(feeOnTransferERC20), amount: defaultAmount }),
+            nonce: 2,
+            deadline: block.timestamp + 100
+        });
+        bytes memory feeSignature =
+            this.getPermitTransferSignature(feePermit, senderKey, address(ics20Transfer), permit2.DOMAIN_SEPARATOR());
+
+        feeOnTransferERC20.mint(sender, defaultAmount);
         vm.prank(sender);
-        ics20Transfer.sendTransferWithPermit2(_getTestSendTransferMsg(), permit, signature);
+        feeOnTransferERC20.approve(address(permit2), defaultAmount);
+
+        vm.expectRevert(
+            abi.encodeWithSelector(IICS20Errors.ICS20UnexpectedERC20Balance.selector, defaultAmount, defaultAmount - 1)
+        );
+        vm.prank(sender);
+        ics20Transfer.sendTransferWithPermit2(msgSendTransfer, feePermit, feeSignature);
     }
 
     function test_success_sendTransferWithSender() public {

test/solidity-ibc/mocks/TestERC20.sol

@@ -63,3 +63,23 @@ contract TestCustomERC20 is ERC20, IMintableAndBurnable {
         _burn(from, amount);
     }
 }
+
+// Test contract to deploy ERC20 with fee on transfer, it takes a fee of 1 unit on every transfer and burns it, so the
+// recipient receives amount - 1
+contract FeeOnTransferERC20 is ERC20 {
+    constructor() ERC20("Fee Token", "FEE") { }
+
+    function mint(address to, uint256 amount) external {
+        _mint(to, amount);
+    }
+
+    function _update(address from, address to, uint256 amount) internal override {
+        if (from != address(0) && to != address(0)) {
+            uint256 fee = 1;
+            super._update(from, address(0), fee); // burn fee
+            super._update(from, to, amount - fee); // deliver remainder
+        } else {
+            super._update(from, to, amount);
+        }
+    }
+}