PIP36: use call over transfer in mrc20

contracts/child/MRC20.sol

@@ -1,11 +1,11 @@
-pragma solidity ^0.5.11;
+pragma solidity 0.5.17;
 
 import "./BaseERC20NoSig.sol";
 
 /**
- * @title Matic token contract
- * @notice This contract is an ECR20 like wrapper over native ether (matic token) transfers on the matic chain
- * @dev ERC20 methods have been made payable while keeping their method signature same as other ChildERC20s on Matic
+ * @title Polygon Ecosystem Token contract
+ * @notice This contract is an ECR20 like wrapper over native gas token transfers on the Polygon PoS chain
+ * @dev ERC20 methods have been made payable while keeping their method signature same as other ChildERC20s on PoS
  */
 contract MRC20 is BaseERC20NoSig {
     event Transfer(address indexed from, address indexed to, uint256 value);
@@ -14,11 +14,17 @@ contract MRC20 is BaseERC20NoSig {
     uint8 private constant DECIMALS = 18;
     bool isInitialized;
 
+    uint256 locked = 0; // append to storage layout
+    modifier nonReentrant() {
+        require(locked == 0, "reentrancy");
+        locked = 1;
+        _;
+        locked = 0;
+    }
+
     constructor() public {}
 
     function initialize(address _childChain, address _token) public {
-        // Todo: once BorValidator(@0x1000) contract added uncomment me
-        // require(msg.sender == address(0x1000));
         require(!isInitialized, "The contract is already initialized");
         isInitialized = true;
         token = _token;
@@ -38,12 +44,11 @@ contract MRC20 is BaseERC20NoSig {
 
         // input balance
         uint256 input1 = balanceOf(user);
+        currentSupply = currentSupply.add(amount);
 
         // transfer amount to user
-        address payable _user = address(uint160(user));
-        _user.transfer(amount);
-
-        currentSupply = currentSupply.add(amount);
+        // not reenterant since this method is only called by commitState on StateReceiver which is onlySystem
+        _nativeTransfer(user, amount);
 
         // deposit events
         emit Deposit(token, user, amount, input1, balanceOf(user));
@@ -66,18 +71,18 @@ contract MRC20 is BaseERC20NoSig {
     }
 
     function name() public pure returns (string memory) {
-        return "Matic Token";
+        return "Polygon Ecosystem Token";
     }
 
     function symbol() public pure returns (string memory) {
-        return "MATIC";
+        return "POL";
     }
 
     function decimals() public pure returns (uint8) {
         return DECIMALS;
     }
 
-    function totalSupply() public view returns (uint256) {
+    function totalSupply() public pure returns (uint256) {
         return 10000000000 * 10**uint256(DECIMALS);
     }
 
@@ -98,13 +103,28 @@ contract MRC20 is BaseERC20NoSig {
 
     /**
    * @dev _transfer is invoked by _transferFrom method that is inherited from BaseERC20.
-   * This enables us to transfer MaticEth between users while keeping the interface same as that of an ERC20 Token.
+   * This enables us to transfer Polygon ETH between users while keeping the interface same as that of an ERC20 Token.
    */
     function _transfer(address sender, address recipient, uint256 amount)
         internal
     {
         require(recipient != address(this), "can't send to MRC20");
-        address(uint160(recipient)).transfer(amount);
+        _nativeTransfer(recipient, amount);
         emit Transfer(sender, recipient, amount);
     }
+
+    // @notice method to transfer native asset to receiver (nonReentrant)
+    // @dev 5000 gas is forwarded in the call to receiver
+    // @dev msg.value checks (if req), emitting logs are handled seperately
+    // @param receiver address to transfer native token to
+    // @param amount amount of native token to transfer
+    function _nativeTransfer(address receiver, uint256 amount) internal nonReentrant {
+        uint256 txGasLimit = 5000;
+        (bool success, bytes memory ret) = receiver.call.value(amount).gas(txGasLimit)("");
+        if (!success) {
+            assembly {
+                revert(add(ret, 0x20), mload(ret)) // bubble up revert
+            }
+        }
+    }
 }

contracts/child/misc/EIP712.sol

@@ -8,8 +8,8 @@ contract LibEIP712Domain is ChainIdMixin {
         abi.encodePacked(EIP712_DOMAIN_SCHEMA)
     );
 
-    string internal constant EIP712_DOMAIN_NAME = "Matic Network";
-    string internal constant EIP712_DOMAIN_VERSION = "1";
+    string internal constant EIP712_DOMAIN_NAME = "Polygon Ecosystem Token";
+    string internal constant EIP712_DOMAIN_VERSION = "2";
     uint256 internal constant EIP712_DOMAIN_CHAINID = CHAINID;
 
     bytes32 public EIP712_DOMAIN_HASH;

contracts/test/NativeTokenReceiver.sol

@@ -0,0 +1,49 @@
+pragma solidity 0.5.17;
+
+contract NativeTokenReceiver_Reverts {
+    function() external payable {
+        revert("!allowed");
+    }
+}
+
+contract NativeTokenReceiver {
+    event SafeReceived(address indexed sender, uint value);
+
+    // bytes32(uint(keccak256("singleton")) - 1)
+    bytes32 public constant SINGLETON_SLOT = 0x3d9111c4ec40e72567dff1e7eb8686c719e04ff7490697118315d2143e8e9edb;
+
+    constructor() public {
+        address receiver = address(new Receive());
+        assembly {
+            sstore(SINGLETON_SLOT, receiver)
+        }
+    }
+
+    function() external payable {
+        assembly {
+            let singleton := sload(SINGLETON_SLOT)
+            calldatacopy(0, 0, calldatasize())
+            let success := delegatecall(gas(), singleton, 0, calldatasize(), 0, 0)
+            if iszero(success) {
+                returndatacopy(0, 0, returndatasize())
+                revert(0, returndatasize())
+            }
+        }
+    }
+}
+
+contract Receive {
+    event SafeReceived(address indexed sender, uint value);
+    function() external payable {
+        emit SafeReceived(msg.sender, msg.value);
+    }
+}
+
+contract NativeTokenReceiver_OOG {
+    uint counter;
+    function() external payable {
+        for (uint i; i < 100; i++) {
+            counter++;
+        }
+    }
+}

test/foundry/ForkupgradeStakeManager.t.sol

@@ -28,9 +28,16 @@ contract ForkupgradeStakeManagerTest is Test, UpgradeStake_DepositManager_Mainne
         Vm.Wallet memory wallet = vm.createWallet("fork wallet");
 
         loadConfig();
-        (StakeManager stakeManagerImpl, ValidatorShare validatorShareImpl, DepositManager depositManagerImpl) = deployImplementations(wallet.privateKey);
-        (bytes memory scheduleBatchPayload, bytes memory executeBatchPayload, bytes32 payloadId) =
-            createPayload(stakeManagerImpl, validatorShareImpl, depositManagerImpl);
+        (
+            StakeManager stakeManagerImpl,
+            ValidatorShare validatorShareImpl,
+            DepositManager depositManagerImpl
+        ) = deployImplementations(wallet.privateKey);
+        (bytes memory scheduleBatchPayload, bytes memory executeBatchPayload, bytes32 payloadId) = createPayload(
+            stakeManagerImpl,
+            validatorShareImpl,
+            depositManagerImpl
+        );
 
         uint256 balanceStakeManager = maticToken.balanceOf(address(stakeManagerProxy));
         console.log("Initial StakeManager Matic balance: ", balanceStakeManager);

test/helpers/artifacts.js

@@ -71,3 +71,7 @@ export const ChildERC721Proxified = await ethers.getContractFactory('ChildERC721
 export const ChildERC721Mintable = await ethers.getContractFactory('ChildERC721Mintable', childSigner)
 export const MRC20 = await ethers.getContractFactory('MRC20', childSigner)
 export const TestMRC20 = await ethers.getContractFactory('TestMRC20', childSigner)
+
+export const NativeTokenReceiver_Reverts = await ethers.getContractFactory('NativeTokenReceiver_Reverts', childSigner)
+export const NativeTokenReceiver_OOG = await ethers.getContractFactory('NativeTokenReceiver_OOG', childSigner)
+export const NativeTokenReceiver = await ethers.getContractFactory('NativeTokenReceiver', childSigner)

test/helpers/marketplaceUtils.js.template

@@ -79,8 +79,8 @@ function getTransferTypedData({
       ]
     },
     domain: {
-      name: "Matic Network",
-      version: "1",
+      name: "Polygon Ecosystem Token",
+      version: "2",
       chainId: {{ borChainId }},
       verifyingContract: tokenAddress
     },

test/helpers/utils.js

@@ -273,7 +273,7 @@ export function fireDepositFromMainToMatic(childChain, eventId, user, tokenAddre
   return childChain.onStateReceive(eventId, encodeDepositStateSync(user, tokenAddress, amountOrToken, depositBlockId))
 }
 
-function encodeDepositStateSync(user, rootToken, tokenIdOrAmount, depositId) {
+export function encodeDepositStateSync(user, rootToken, tokenIdOrAmount, depositId) {
   if (typeof tokenIdOrAmount !== 'string') {
     tokenIdOrAmount = tokenIdOrAmount.toString()
   }

test/integration/root/DepositManager.test.js

@@ -2,12 +2,13 @@ import * as chai from 'chai'
 import chaiAsPromised from 'chai-as-promised'
 import deployer from '../../helpers/deployer.js'
 import * as utils from '../../helpers/utils.js'
+import * as contractFactories from '../../helpers/artifacts.js'
 import crypto from 'crypto'
 
 chai.use(chaiAsPromised).should()
 
 describe('DepositManager @skip-on-coverage', async function (accounts) {
-  let depositManager, childContracts
+  let depositManager, childContracts, maticE20
   const amount = web3.utils.toBN('10').pow(web3.utils.toBN('18'))
 
   describe('deposits on root and child', async function () {
@@ -22,6 +23,7 @@ describe('DepositManager @skip-on-coverage', async function (accounts) {
       const contracts = await deployer.freshDeploy(accounts[0])
       depositManager = contracts.depositManager
       childContracts = await deployer.initializeChildChain()
+      maticE20 = await deployer.deployMaticToken()
     })
 
     it('depositERC20', async function () {
@@ -38,17 +40,104 @@ describe('DepositManager @skip-on-coverage', async function (accounts) {
       utils.assertBigNumberEquality(balance, amount)
     })
 
-    it('deposit Matic Tokens', async function () {
-      const bob = '0x' + crypto.randomBytes(20).toString('hex')
-      const e20 = await deployer.deployMaticToken()
-      utils.assertBigNumberEquality(await e20.childToken.balanceOf(bob), 0)
-      await utils.deposit(depositManager, childContracts.childChain, e20.rootERC20, bob, amount, {
-        rootDeposit: true,
-        erc20: true
+    describe('Matic Tokens', async function () {
+      it('deposit to EOA', async function () {
+        const bob = '0x' + crypto.randomBytes(20).toString('hex')
+        utils.assertBigNumberEquality(await maticE20.childToken.balanceOf(bob), 0)
+        await utils.deposit(depositManager, childContracts.childChain, maticE20.rootERC20, bob, amount, {
+          rootDeposit: true,
+          erc20: true
+        })
+
+        // assert deposit on child chain
+        utils.assertBigNumberEquality(await maticE20.childToken.balanceOf(bob), amount)
       })
 
-      // assert deposit on child chain
-      utils.assertBigNumberEquality(await e20.childToken.balanceOf(bob), amount)
+      it('deposit to non EOA', async function () {
+        const scwReceiver = await contractFactories.NativeTokenReceiver.deploy()
+        utils.assertBigNumberEquality(await maticE20.childToken.balanceOf(scwReceiver.address), 0)
+        const stateSyncTxn = await utils.deposit(
+          depositManager,
+          childContracts.childChain,
+          maticE20.rootERC20,
+          scwReceiver.address,
+          amount,
+          {
+            rootDeposit: true,
+            erc20: true
+          }
+        )
+
+        utils.assertInTransaction(stateSyncTxn, contractFactories.NativeTokenReceiver, 'SafeReceived', {
+          sender: maticE20.childToken.address,
+          value: amount.toString()
+        })
+
+        // assert deposit on child chain
+        utils.assertBigNumberEquality(await maticE20.childToken.balanceOf(scwReceiver.address), amount)
+      })
+
+      it('deposit to reverting non EOA', async function () {
+        const scwReceiver_Reverts = await contractFactories.NativeTokenReceiver_Reverts.deploy()
+        utils.assertBigNumberEquality(await maticE20.childToken.balanceOf(scwReceiver_Reverts.address), 0)
+        const newDepositBlockEvent = await utils.depositOnRoot(
+          depositManager,
+          maticE20.rootERC20,
+          scwReceiver_Reverts.address,
+          amount.toString(),
+          {
+            rootDeposit: true,
+            erc20: true
+          }
+        )
+        try {
+          const tx = await childContracts.childChain.onStateReceive(
+            '0xf' /* dummy id */,
+            utils.encodeDepositStateSync(
+              scwReceiver_Reverts.address,
+              maticE20.rootERC20.address,
+              amount,
+              newDepositBlockEvent.args.depositBlockId
+            )
+          )
+          await tx.wait()
+        } catch (error) {
+          // problem with return data decoding on bor rpc & hh
+          chai.assert(error.message.includes('transaction failed'), 'Transaction should have failed')
+        }
+        // assert deposit did not happen on child chain
+        utils.assertBigNumberEquality(await maticE20.childToken.balanceOf(scwReceiver_Reverts.address), 0)
+      })
+
+      it('deposit to reverting with OOG', async function () {
+        const scwReceiver_OOG = await contractFactories.NativeTokenReceiver_OOG.deploy()
+        utils.assertBigNumberEquality(await maticE20.childToken.balanceOf(scwReceiver_OOG.address), 0)
+        const newDepositBlockEvent = await utils.depositOnRoot(
+          depositManager,
+          maticE20.rootERC20,
+          scwReceiver_OOG.address,
+          amount.toString(),
+          {
+            rootDeposit: true,
+            erc20: true
+          }
+        )
+        try {
+          const tx = await childContracts.childChain.onStateReceive(
+            '0xb' /* dummy id */,
+            utils.encodeDepositStateSync(
+              scwReceiver_OOG.address,
+              maticE20.rootERC20.address,
+              amount,
+              newDepositBlockEvent.args.depositBlockId
+            )
+          )
+          await tx.wait()
+        } catch (error) {
+          chai.assert(error.message.includes('transaction failed'), 'Transaction should have failed')
+        }
+        utils.assertBigNumberEquality(await maticE20.childToken.balanceOf(scwReceiver_OOG.address), 0)
+      })
     })
   })
 })