PIP36: use call over transfer in mrc20

.github/workflows/ci.yml

@@ -2,9 +2,9 @@ name: CI
 
 on:
   push:
-    branches:
-      - main
+    branches: [main, dev]
   pull_request:
+    branches: [main, dev]
 
 jobs:
   build:
@@ -47,6 +47,5 @@ jobs:
         id: build
 
       - name: Run Forge tests
-        run: |
-          forge test -vvv
+        run: if test -d test/foundry && find test/foundry -name '*.sol' | grep -q .; then forge test -vvv; else echo "No .sol files found in test/foundry directory. Skipping Forge tests."; fi
         id: test

contracts/child/MRC20.sol

@@ -1,4 +1,4 @@
-pragma solidity ^0.5.11;
+pragma solidity 0.5.17;
 
 import "./BaseERC20NoSig.sol";
 
@@ -14,6 +14,14 @@ contract MRC20 is BaseERC20NoSig {
     uint8 private constant DECIMALS = 18;
     bool isInitialized;
 
+    uint256 locked = 1; // append to storage layout
+    modifier nonReentrant() {
+        require(locked == 1, "reentrancy");
+        locked = 2;
+        _;
+        locked = 1;
+    }
+
     constructor() public {}
 
     function initialize(address _childChain, address _token) public {
@@ -29,7 +37,7 @@ contract MRC20 is BaseERC20NoSig {
         revert("Disabled feature");
     }
 
-    function deposit(address user, uint256 amount) public onlyOwner {
+    function deposit(address user, uint256 amount) public onlyOwner nonReentrant {
         // check for amount and user
         require(
             amount > 0 && user != address(0x0),
@@ -38,12 +46,18 @@ 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);
+        uint256 txGasLimit = 5000;
+        assembly {
+            // not reenterant since this method is only called by commitState on StateReceiver which is onlySystem
+            let success := call(txGasLimit, user, amount, 0, 0, 0, 0)
+            if iszero(success) {
+                returndatacopy(0, 0, returndatasize()) // bubble up revert
+                revert(0, returndatasize())
+            }
+        }
 
         // deposit events
         emit Deposit(token, user, amount, input1, balanceOf(user));

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/helpers/artifacts.js

@@ -69,3 +69,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/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)
+      })
     })
   })
 })