From e44f4d8bf4fe665c784c7071a77ce635f41b9558 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Sat, 21 Aug 2021 00:09:57 +0300 Subject: [PATCH] Block ERC20 selectors in AMB requests (#630) --- .../arbitrary_message/BasicAMB.sol | 9 ++++ .../arbitrary_message/BasicHomeAMB.sol | 2 +- .../arbitrary_message/MessageDelivery.sol | 26 ++++++++++- test/arbitrary_message/foreign_bridge.test.js | 44 +++++++++++++++++++ 4 files changed, 78 insertions(+), 3 deletions(-) diff --git a/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol b/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol index 1629567d2..6a8b11d05 100644 --- 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 index fde2cbae3..e8badae71 100644 --- 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 index 78d859c90..1bff1d607 100644 --- 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 index e12723e69..e37507890 100644 --- 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