Block ERC20 selectors in AMB requests (omni#630)

gnosischain · Aug 20, 2021 · e44f4d8 · e44f4d8
1 parent 39f5835
commit e44f4d8
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 3 deletions.
diff --git a/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol b/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol
@@ -101,6 +101,15 @@ contract BasicAMB is BasicBridge, VersionableAMB {
         return boolStorage[ALLOW_REENTRANT_REQUESTS];
     }
 
+    /**
+     * @dev Withdraws the erc20 tokens or native coins from this contract.
+     * @param _token address of the claimed token or address(0) for native coins.
+     * @param _to address of the tokens/coins receiver.
+     */
+    function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner {
+        claimValues(_token, _to);
+    }
+
     /**
      * Internal function for retrieving current nonce value
      * @return nonce value

diff --git a/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol b/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol
@@ -45,7 +45,7 @@ contract BasicHomeAMB is BasicAMB, MessageDelivery {
     * @param _data calldata passed to the executor on the other side.
     * @param _gas gas limit used on the other network for executing a message.
     */
-    function requireToConfirmMessage(address _contract, bytes _data, uint256 _gas) external returns (bytes32) {
+    function requireToConfirmMessage(address _contract, bytes memory _data, uint256 _gas) public returns (bytes32) {
         return _sendMessage(_contract, _data, _gas, SEND_TO_MANUAL_LANE);
     }
 

diff --git a/contracts/upgradeable_contracts/arbitrary_message/MessageDelivery.sol b/contracts/upgradeable_contracts/arbitrary_message/MessageDelivery.sol
@@ -19,7 +19,7 @@ contract MessageDelivery is BasicAMB, MessageProcessor {
     * @param _data calldata passed to the executor on the other side
     * @param _gas gas limit used on the other network for executing a message
     */
-    function requireToPassMessage(address _contract, bytes _data, uint256 _gas) public returns (bytes32) {
+    function requireToPassMessage(address _contract, bytes memory _data, uint256 _gas) public returns (bytes32) {
         return _sendMessage(_contract, _data, _gas, SEND_TO_ORACLE_DRIVEN_LANE);
     }
 
@@ -30,12 +30,34 @@ contract MessageDelivery is BasicAMB, MessageProcessor {
     * @param _gas gas limit used on the other network for executing a message
     * @param _dataType AMB message dataType to be included as a part of the header
     */
-    function _sendMessage(address _contract, bytes _data, uint256 _gas, uint256 _dataType) internal returns (bytes32) {
+    function _sendMessage(address _contract, bytes memory _data, uint256 _gas, uint256 _dataType)
+        internal
+        returns (bytes32)
+    {
         // it is not allowed to pass messages while other messages are processed
         // if other is not explicitly configured
         require(messageId() == bytes32(0) || allowReentrantRequests());
         require(_gas >= MIN_GAS_PER_CALL && _gas <= maxGasPerTx());
 
+        uint256 selector;
+        assembly {
+            selector := and(mload(add(_data, 4)), 0xffffffff)
+        }
+        // In order to prevent possible unauthorized ERC20 withdrawals, the following function signatures are prohibited:
+        // * transfer(address,uint256)
+        // * approve(address,uint256)
+        // * transferFrom(address,address,uint256)
+        // * approveAndCall(address,uint256,bytes)
+        // * transferAndCall(address,uint256,bytes)
+        // See https://medium.com/immunefi/xdai-stake-arbitrary-call-method-bug-postmortem-f80a90ac56e3 for more details
+        require(
+            selector != 0xa9059cbb &&
+                selector != 0x095ea7b3 &&
+                selector != 0x23b872dd &&
+                selector != 0x4000aea0 &&
+                selector != 0xcae9ca51
+        );
+
         (bytes32 _messageId, bytes memory header) = _packHeader(_contract, _gas, _dataType);
 
         bytes memory eventData = abi.encodePacked(header, _data);

diff --git a/test/arbitrary_message/foreign_bridge.test.js b/test/arbitrary_message/foreign_bridge.test.js
@@ -3,6 +3,7 @@ const HomeBridge = artifacts.require('HomeAMB.sol')
 const GasToken = artifacts.require('GasTokenMock.sol')
 const BridgeValidators = artifacts.require('BridgeValidators.sol')
 const Box = artifacts.require('Box.sol')
+const ERC20Mock = artifacts.require('ERC20Mock.sol')
 const ERC677ReceiverTest = artifacts.require('ERC677ReceiverTest.sol')
 const EternalStorageProxy = artifacts.require('EternalStorageProxy.sol')
 
@@ -334,6 +335,49 @@ contract('ForeignAMB', async accounts => {
       expect(messageId2).to.include(`${bridgeId}0000000000000001`)
       expect(messageId3).to.include(`${bridgeId}0000000000000002`)
     })
+    it('should fail to send message with blocked signatures', async () => {
+      const blockedFunctions = [
+        'transfer(address,uint256)',
+        'approve(address,uint256)',
+        'transferFrom(address,address,uint256)',
+        'approveAndCall(address,uint256,bytes)',
+        'transferAndCall(address,uint256,bytes)'
+      ].map(web3.eth.abi.encodeFunctionSignature)
+      for (const signature of blockedFunctions) {
+        await foreignBridge.requireToPassMessage(accounts[7], signature, 10000).should.be.rejected
+      }
+      await foreignBridge.requireToPassMessage(accounts[7], '0x11223344', 10000).should.be.fulfilled
+    })
+  })
+  describe('claimTokens', () => {
+    let foreignBridge
+    let token
+    beforeEach(async () => {
+      const foreignBridgeV1 = await ForeignBridge.new()
+      token = await ERC20Mock.new('Test', 'TST', 18)
+
+      // create proxy
+      const proxy = await EternalStorageProxy.new()
+      await proxy.upgradeTo('1', foreignBridgeV1.address).should.be.fulfilled
+
+      foreignBridge = await ForeignBridge.at(proxy.address)
+      await foreignBridge.initialize(
+        FOREIGN_CHAIN_ID_HEX,
+        HOME_CHAIN_ID_HEX,
+        validatorContract.address,
+        oneEther,
+        gasPrice,
+        requiredBlockConfirmations,
+        owner
+      )
+    })
+    it('should claim mistakenly locked tokens', async () => {
+      await token.mint(foreignBridge.address, oneEther)
+      await foreignBridge.claimTokens(token.address, owner, { from: accounts[2] }).should.be.rejected
+      await foreignBridge.claimTokens(token.address, owner, { from: owner }).should.be.fulfilled
+
+      expect(await token.balanceOf(owner)).to.be.bignumber.equal(oneEther)
+    })
   })
   describe('executeSignatures', () => {
     let foreignBridge