From 00a5b1436c26b677c98b41aae1e83067fb1930f6 Mon Sep 17 00:00:00 2001 From: Guilherme Dantas Date: Tue, 14 Nov 2023 15:57:43 -0300 Subject: [PATCH] feat!: revert on failed ERC-20 deposit --- .../rollups/.changeset/tough-shoes-taste.md | 7 + .../rollups/contracts/portals/ERC20Portal.sol | 7 +- .../test/foundry/portals/ERC20Portal.t.sol | 127 ++++++++++++------ 3 files changed, 97 insertions(+), 44 deletions(-) create mode 100644 onchain/rollups/.changeset/tough-shoes-taste.md diff --git a/onchain/rollups/.changeset/tough-shoes-taste.md b/onchain/rollups/.changeset/tough-shoes-taste.md new file mode 100644 index 00000000..6b95c719 --- /dev/null +++ b/onchain/rollups/.changeset/tough-shoes-taste.md @@ -0,0 +1,7 @@ +--- +"@cartesi/rollups": major +--- + +Changed the ERC-20 portal to revert whenever `transferFrom` returns `false`. +This change was made to prevent DApp back-end developers from accepting failed transfers by not checking the `success` flag of ERC-20 deposit inputs. +We used OpenZeppelin's `SafeERC20` to deliver an even safer and user-friendly experience through the ERC-20 portal. diff --git a/onchain/rollups/contracts/portals/ERC20Portal.sol b/onchain/rollups/contracts/portals/ERC20Portal.sol index 561714eb..b8a7aa2b 100644 --- a/onchain/rollups/contracts/portals/ERC20Portal.sol +++ b/onchain/rollups/contracts/portals/ERC20Portal.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.8; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import {IERC20Portal} from "./IERC20Portal.sol"; import {InputRelay} from "../inputs/InputRelay.sol"; @@ -15,6 +16,8 @@ import {InputEncoding} from "../common/InputEncoding.sol"; /// @notice This contract allows anyone to perform transfers of /// ERC-20 tokens to a DApp while informing the off-chain machine. contract ERC20Portal is InputRelay, IERC20Portal { + using SafeERC20 for IERC20; + /// @notice Constructs the portal. /// @param _inputBox The input box used by the portal constructor(IInputBox _inputBox) InputRelay(_inputBox) {} @@ -25,10 +28,10 @@ contract ERC20Portal is InputRelay, IERC20Portal { uint256 _amount, bytes calldata _execLayerData ) external override { - bool success = _token.transferFrom(msg.sender, _dapp, _amount); + _token.safeTransferFrom(msg.sender, _dapp, _amount); bytes memory input = InputEncoding.encodeERC20Deposit( - success, + true, _token, msg.sender, _amount, diff --git a/onchain/rollups/test/foundry/portals/ERC20Portal.t.sol b/onchain/rollups/test/foundry/portals/ERC20Portal.t.sol index fdaf70ec..8c998db9 100644 --- a/onchain/rollups/test/foundry/portals/ERC20Portal.t.sol +++ b/onchain/rollups/test/foundry/portals/ERC20Portal.t.sol @@ -11,17 +11,22 @@ import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {IInputBox} from "contracts/inputs/IInputBox.sol"; import {InputBox} from "contracts/inputs/InputBox.sol"; -import {InputEncoding} from "contracts/common/InputEncoding.sol"; contract NormalToken is ERC20 { - constructor(uint256 _initialSupply) ERC20("NormalToken", "NORMAL") { - _mint(msg.sender, _initialSupply); + constructor( + address _tokenOwner, + uint256 _initialSupply + ) ERC20("NormalToken", "NORMAL") { + _mint(_tokenOwner, _initialSupply); } } contract UntransferableToken is ERC20 { - constructor(uint256 _initialSupply) ERC20("UntransferableToken", "UTFAB") { - _mint(msg.sender, _initialSupply); + constructor( + address _tokenOwner, + uint256 _initialSupply + ) ERC20("UntransferableToken", "UTFAB") { + _mint(_tokenOwner, _initialSupply); } function transfer(address, uint256) public pure override returns (bool) { @@ -37,6 +42,27 @@ contract UntransferableToken is ERC20 { } } +contract RevertingToken is ERC20 { + constructor( + address _tokenOwner, + uint256 _initialSupply + ) ERC20("RevertingToken", "REVERT") { + _mint(_tokenOwner, _initialSupply); + } + + function transfer(address, uint256) public pure override returns (bool) { + revert(); + } + + function transferFrom( + address, + address, + uint256 + ) public pure override returns (bool) { + revert(); + } +} + contract WatcherToken is ERC20 { IInputBox inputBox; @@ -49,10 +75,11 @@ contract WatcherToken is ERC20 { constructor( IInputBox _inputBox, + address _tokenOwner, uint256 _initialSupply ) ERC20("WatcherToken", "WTCHR") { inputBox = _inputBox; - _mint(msg.sender, _initialSupply); + _mint(_tokenOwner, _initialSupply); } function transfer( @@ -114,12 +141,9 @@ contract ERC20PortalTest is Test { assertEq(address(portal.getInputBox()), address(inputBox)); } - function testERC20DepositTrue( - uint256 _amount, - bytes calldata _data - ) public { + function testERC20Deposit(uint256 _amount, bytes calldata _data) public { // Create a normal token - token = new NormalToken(_amount); + token = new NormalToken(alice, _amount); // Construct the ERC-20 deposit input bytes memory input = abi.encodePacked( @@ -130,8 +154,6 @@ contract ERC20PortalTest is Test { _data ); - // Transfer ERC-20 tokens to Alice and start impersonating her - require(token.transfer(alice, _amount), "token transfer fail"); vm.startPrank(alice); // Allow the portal to withdraw `_amount` tokens from Alice @@ -159,45 +181,72 @@ contract ERC20PortalTest is Test { assertEq(inputBox.getNumberOfInputs(dapp), 1); } - function testERC20DepositFalse( + function testRevertsOperationDidNotSucceed( uint256 _amount, bytes calldata _data ) public { // Create untransferable token - token = new UntransferableToken(_amount); + token = new UntransferableToken(alice, _amount); - // Construct the ERC-20 deposit input - bytes memory input = abi.encodePacked( - false, - token, - alice, - _amount, - _data - ); - - // Start impersonating Alice vm.startPrank(alice); + token.approve(address(portal), _amount); + // Save the ERC-20 token balances uint256 aliceBalanceBefore = token.balanceOf(alice); uint256 dappBalanceBefore = token.balanceOf(dapp); uint256 portalBalanceBefore = token.balanceOf(address(portal)); - // Expect InputAdded to be emitted with the right arguments - vm.expectEmit(true, true, false, true, address(inputBox)); - emit InputAdded(dapp, 0, address(portal), input); - // Transfer ERC-20 tokens to the DApp via the portal + vm.expectRevert("SafeERC20: ERC20 operation did not succeed"); portal.depositERC20Tokens(token, dapp, _amount, _data); vm.stopPrank(); - // Check the balances after the deposit + // Same balances as before assertEq(token.balanceOf(alice), aliceBalanceBefore); assertEq(token.balanceOf(dapp), dappBalanceBefore); assertEq(token.balanceOf(address(portal)), portalBalanceBefore); - // Check the DApp's input box - assertEq(inputBox.getNumberOfInputs(dapp), 1); + // No input added + assertEq(inputBox.getNumberOfInputs(dapp), 0); + } + + function testRevertsNonContract( + uint256 _amount, + bytes calldata _data + ) public { + // Use an EOA as token + token = IERC20(vm.addr(3)); + + vm.startPrank(alice); + + // Transfer ERC-20 tokens to the DApp via the portal + vm.expectRevert("Address: call to non-contract"); + portal.depositERC20Tokens(token, dapp, _amount, _data); + vm.stopPrank(); + + // No input added + assertEq(inputBox.getNumberOfInputs(dapp), 0); + } + + function testRevertsLowLevelCallFailed( + uint256 _amount, + bytes calldata _data + ) public { + // Create bad token + token = new RevertingToken(alice, _amount); + + vm.startPrank(alice); + + token.approve(address(portal), _amount); + + // Transfer ERC-20 tokens to the DApp via the portal + vm.expectRevert("SafeERC20: low-level call failed"); + portal.depositERC20Tokens(token, dapp, _amount, _data); + vm.stopPrank(); + + // No input added + assertEq(inputBox.getNumberOfInputs(dapp), 0); } function testRevertsInsufficientAllowance( @@ -208,10 +257,8 @@ contract ERC20PortalTest is Test { vm.assume(_amount > 0); // Create a normal token - token = new NormalToken(_amount); + token = new NormalToken(alice, _amount); - // Transfer ERC-20 tokens to Alice and start impersonating her - require(token.transfer(alice, _amount), "token transfer fail"); vm.startPrank(alice); // Expect deposit to revert with message @@ -231,10 +278,8 @@ contract ERC20PortalTest is Test { vm.assume(_amount < type(uint256).max); // Create a normal token - token = new NormalToken(_amount); + token = new NormalToken(alice, _amount); - // Transfer ERC-20 tokens to Alice and start impersonating her - require(token.transfer(alice, _amount), "token transfer fail"); vm.startPrank(alice); // Allow the portal to withdraw `_amount+1` tokens from Alice @@ -251,10 +296,8 @@ contract ERC20PortalTest is Test { function testNumberOfInputs(uint256 _amount, bytes calldata _data) public { // Create a token that records the number of inputs it has received - token = new WatcherToken(inputBox, _amount); + token = new WatcherToken(inputBox, alice, _amount); - // Transfer ERC-20 tokens to Alice and start impersonating her - require(token.transfer(alice, _amount), "token transfer fail"); vm.startPrank(alice); // Allow the portal to withdraw `_amount` tokens from Alice @@ -349,7 +392,7 @@ contract ERC20PortalInvariantTest is Test { function setUp() public { inputBox = new InputBox(); portal = new ERC20Portal(inputBox); - token = new NormalToken(tokenSupply); + token = new NormalToken(address(this), tokenSupply); handler = new ERC20PortalHandler(portal, token); // transfer all tokens to handler require(