Revert transactions when message processing fails (#240)

asaj/sdk-proposal
Asa Oines 3 years ago committed by GitHub
parent 9527f362d4
commit 2899d965fd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 86
      solidity/abacus-core/contracts/Inbox.sol
  2. 13
      solidity/abacus-core/contracts/test/TestInbox.sol
  3. 16
      solidity/abacus-core/contracts/test/bad-recipient/BadRecipient2.sol
  4. 17
      solidity/abacus-core/contracts/test/bad-recipient/BadRecipient4.sol
  5. 52
      solidity/abacus-core/test/inbox.test.ts
  6. 5
      solidity/abacus-core/test/lib/AbacusDeployment.ts
  7. 14
      solidity/abacus-core/test/xAppConnectionManager.test.ts
  8. 3
      solidity/abacus-xapps/test/governance/governanceRouter.test.ts
  9. 2
      typescript/abacus-deploy/config/environments/dev/core.ts
  10. 2
      typescript/abacus-deploy/config/environments/local/core.ts
  11. 2
      typescript/abacus-deploy/config/environments/mainnet/core.ts
  12. 2
      typescript/abacus-deploy/config/environments/testnet-legacy/core.ts
  13. 2
      typescript/abacus-deploy/config/environments/testnet/core.ts
  14. 10
      typescript/abacus-deploy/scripts/deploy-inbox.ts
  15. 3
      typescript/abacus-deploy/src/config/core.ts
  16. 2
      typescript/abacus-deploy/src/core/CoreInstance.ts
  17. 7
      typescript/abacus-deploy/src/core/implementation.ts
  18. 3
      typescript/abacus-deploy/src/core/types.ts
  19. 2
      typescript/abacus-deploy/test/inputs.ts
  20. 5
      typescript/hardhat/src/TestAbacusDeploy.ts

@ -13,8 +13,8 @@ import {TypedMemView} from "@summa-tx/memview-sol/contracts/TypedMemView.sol";
/**
* @title Inbox
* @author Celo Labs Inc.
* @notice Track root updates on Outbox,
* prove and dispatch messages to end recipients.
* @notice Track root updates on Outbox, prove and dispatch messages to end
* recipients.
*/
contract Inbox is Version0, Common {
// ============ Libraries ============
@ -36,13 +36,6 @@ contract Inbox is Version0, Common {
Processed
}
// ============ Immutables ============
// Minimum gas for message processing
uint256 public immutable PROCESS_GAS;
// Reserved gas (to ensure tx completes in case message processing runs out)
uint256 public immutable RESERVE_GAS;
// ============ Public Storage ============
// Domain of outbox chain
@ -62,28 +55,13 @@ contract Inbox is Version0, Common {
/**
* @notice Emitted when message is processed
* @param messageHash Hash of message that failed to process
* @param success TRUE if the call was executed successfully, FALSE if the call reverted
* @param returnData the return data from the external call
*/
event Process(
bytes32 indexed messageHash,
bool indexed success,
bytes indexed returnData
);
event Process(bytes32 indexed messageHash);
// ============ Constructor ============
// solhint-disable-next-line no-empty-blocks
constructor(
uint32 _localDomain,
uint256 _processGas,
uint256 _reserveGas
) Common(_localDomain) {
require(_processGas >= 850_000, "!process gas");
require(_reserveGas >= 15_000, "!reserve gas");
PROCESS_GAS = _processGas;
RESERVE_GAS = _reserveGas;
}
constructor(uint32 _localDomain) Common(_localDomain) {}
// ============ Initializer ============
@ -153,12 +131,10 @@ contract Inbox is Version0, Common {
* message payload to end recipient.
* @dev Recipient must implement a `handle` method (refer to IMessageRecipient.sol)
* Reverts if formatted message's destination domain is not the Inbox's domain,
* if message has not been proven,
* or if not enough gas is provided for the dispatch transaction.
* if message has not been proven, or if the dispatch transaction fails.
* @param _message Formatted message
* @return _success TRUE iff dispatch transaction succeeded
*/
function process(bytes memory _message) public returns (bool _success) {
function process(bytes memory _message) public {
bytes29 _m = _message.ref(0);
// ensure message was meant for this domain
require(_m.destination() == localDomain, "!destination");
@ -170,54 +146,10 @@ contract Inbox is Version0, Common {
entered = 0;
// update message status as processed
messages[_messageHash] = MessageStatus.Processed;
// A call running out of gas TYPICALLY errors the whole tx. We want to
// a) ensure the call has a sufficient amount of gas to make a
// meaningful state change.
// b) ensure that if the subcall runs out of gas, that the tx as a whole
// does not revert (i.e. we still mark the message processed)
// To do this, we require that we have enough gas to process
// and still return. We then delegate only the minimum processing gas.
require(gasleft() >= PROCESS_GAS + RESERVE_GAS, "!gas");
// get the message recipient
address _recipient = _m.recipientAddress();
// set up for assembly call
uint256 _toCopy;
uint256 _maxCopy = 256;
uint256 _gas = PROCESS_GAS;
// allocate memory for returndata
bytes memory _returnData = new bytes(_maxCopy);
bytes memory _calldata = abi.encodeWithSignature(
"handle(uint32,bytes32,bytes)",
_m.origin(),
_m.sender(),
_m.body().clone()
);
// dispatch message to recipient
// by assembly calling "handle" function
// we call via assembly to avoid memcopying a very large returndata
// returned by a malicious contract
assembly {
_success := call(
_gas, // gas
_recipient, // recipient
0, // ether value
add(_calldata, 0x20), // inloc
mload(_calldata), // inlen
0, // outloc
0 // outlen
)
// limit our copy to 256 bytes
_toCopy := returndatasize()
if gt(_toCopy, _maxCopy) {
_toCopy := _maxCopy
}
// Store the length of the copied bytes
mstore(_returnData, _toCopy)
// copy the bytes from returndata[0:_toCopy]
returndatacopy(add(_returnData, 0x20), 0, _toCopy)
}
IMessageRecipient _recipient = IMessageRecipient(_m.recipientAddress());
_recipient.handle(_m.origin(), _m.sender(), _m.body().clone());
// emit process results
emit Process(_messageHash, _success, _returnData);
emit Process(_messageHash);
// reset re-entrancy guard
entered = 1;
}

@ -8,11 +8,7 @@ contract TestInbox is Inbox {
using TypedMemView for bytes29;
using Message for bytes29;
constructor(
uint32 _localDomain,
uint256,
uint256
) Inbox(_localDomain, 850_000, 15_000) {} // solhint-disable-line no-empty-blocks
constructor(uint32 _localDomain) Inbox(_localDomain) {} // solhint-disable-line no-empty-blocks
function setMessageProven(bytes memory _message) external {
bytes29 _m = _message.ref(0);
@ -31,11 +27,8 @@ contract TestInbox is Inbox {
return MerkleLib.branchRoot(leaf, proof, index);
}
function testProcess(bytes memory _message)
external
returns (bool _success)
{
(_success) = process(_message);
function testProcess(bytes memory _message) external {
process(_message);
}
function getRevertMsg(bytes memory _res)

@ -1,16 +0,0 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity >=0.6.11;
import {IMessageRecipient} from "../../../interfaces/IMessageRecipient.sol";
contract BadRecipient2 is IMessageRecipient {
function handle(
uint32,
bytes32,
bytes memory
) external pure override {
assembly {
return(0, 0)
}
}
}

@ -1,17 +0,0 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity >=0.6.11;
import {IMessageRecipient} from "../../../interfaces/IMessageRecipient.sol";
contract BadRecipient4 is IMessageRecipient {
function handle(
uint32,
bytes32,
bytes memory
) external pure override {
assembly {
mstore(0, 0xabcdef)
return(0, 32)
}
}
}

@ -5,9 +5,7 @@ import { types, utils } from '@abacus-network/utils';
import { Validator } from './lib/core';
import {
BadRecipient1__factory,
BadRecipient2__factory,
BadRecipient3__factory,
BadRecipient4__factory,
BadRecipient5__factory,
BadRecipient6__factory,
BadRecipientHandle__factory,
@ -23,15 +21,11 @@ const proveAndProcessTestCases = require('../../../vectors/proveAndProcess.json'
const localDomain = 2000;
const remoteDomain = 1000;
const processGas = 850000;
const reserveGas = 15000;
describe('Inbox', async () => {
const badRecipientFactories = [
BadRecipient1__factory,
BadRecipient2__factory,
BadRecipient3__factory,
BadRecipient4__factory,
BadRecipient5__factory,
BadRecipient6__factory,
];
@ -55,7 +49,7 @@ describe('Inbox', async () => {
beforeEach(async () => {
const inboxFactory = new TestInbox__factory(signer);
inbox = await inboxFactory.deploy(localDomain, processGas, reserveGas);
inbox = await inboxFactory.deploy(localDomain);
await inbox.initialize(
remoteDomain,
validatorManager.address,
@ -183,14 +177,10 @@ describe('Inbox', async () => {
// Set message status to types.MessageStatus.Pending
await inbox.setMessageProven(abacusMessage);
// Ensure proper static call return value
const success = await inbox.callStatic.process(abacusMessage);
expect(success).to.be.true;
const processTx = inbox.process(abacusMessage);
await expect(processTx)
.to.emit(inbox, 'Process')
.withArgs(utils.messageHash(abacusMessage), true, '0x');
.withArgs(utils.messageHash(abacusMessage));
});
it('Fails to process an unproved message', async () => {
@ -211,7 +201,7 @@ describe('Inbox', async () => {
});
for (let i = 0; i < badRecipientFactories.length; i++) {
it(`Processes a message from a badly implemented recipient (${
it(`Fails to process a message for a badly implemented recipient (${
i + 1
})`, async () => {
const sender = abacusMessageSender;
@ -230,7 +220,7 @@ describe('Inbox', async () => {
// Set message status to MessageStatus.Pending
await inbox.setMessageProven(abacusMessage);
await inbox.process(abacusMessage);
await expect(inbox.process(abacusMessage)).to.be.reverted;
});
}
@ -254,7 +244,7 @@ describe('Inbox', async () => {
);
});
it('Processes message sent to a non-existent contract address', async () => {
it('Fails to process message sent to a non-existent contract address', async () => {
const nonce = 0;
const body = ethers.utils.formatBytes32String('message');
@ -269,33 +259,10 @@ describe('Inbox', async () => {
// Set message status to types.MessageStatus.Pending
await inbox.setMessageProven(abacusMessage);
await expect(inbox.process(abacusMessage)).to.not.be.reverted;
});
it('Fails to process an undergased transaction', async () => {
const [sender, recipient] = await ethers.getSigners();
const nonce = 0;
const body = ethers.utils.formatBytes32String('message');
const abacusMessage = utils.formatMessage(
remoteDomain,
sender.address,
nonce,
localDomain,
recipient.address,
body,
);
// Set message status to MessageStatus.Pending
await inbox.setMessageProven(abacusMessage);
// Required gas is >= 510,000 (we provide 500,000)
await expect(
inbox.process(abacusMessage, { gasLimit: 500000 }),
).to.be.revertedWith('!gas');
await expect(inbox.process(abacusMessage)).to.be.reverted;
});
it('Returns false when processing message for bad handler function', async () => {
it('Fails to process a message for bad handler function', async () => {
const sender = abacusMessageSender;
const [recipient] = await ethers.getSigners();
const factory = new BadRecipientHandle__factory(recipient);
@ -314,9 +281,8 @@ describe('Inbox', async () => {
// Set message status to MessageStatus.Pending
await inbox.setMessageProven(abacusMessage);
// Ensure bad handler function causes process to return false
let success = await inbox.callStatic.process(abacusMessage);
expect(success).to.be.false;
// Ensure bad handler function causes process to fail
await expect(inbox.process(abacusMessage)).to.be.reverted;
});
it('Proves and processes a message', async () => {

@ -27,9 +27,6 @@ export interface AbacusInstance {
inboxs: Record<number, TestInbox>;
}
const processGas = 850000;
const reserveGas = 15000;
export class AbacusDeployment {
constructor(
public readonly domains: types.Domain[],
@ -81,7 +78,7 @@ export class AbacusDeployment {
const inboxFactory = new TestInbox__factory(signer);
const inboxs: Record<number, TestInbox> = {};
const deploys = remotes.map(async (remoteDomain) => {
const inbox = await inboxFactory.deploy(local, processGas, reserveGas);
const inbox = await inboxFactory.deploy(local);
await inbox.initialize(
remoteDomain,
validatorManager.address,

@ -13,8 +13,6 @@ import {
const ONLY_OWNER_REVERT_MSG = 'Ownable: caller is not the owner';
const localDomain = 1000;
const remoteDomain = 2000;
const processGas = 850000;
const reserveGas = 15000;
describe('XAppConnectionManager', async () => {
let connectionManager: XAppConnectionManager,
@ -30,11 +28,7 @@ describe('XAppConnectionManager', async () => {
const outbox = await outboxFactory.deploy(localDomain);
const inboxFactory = new TestInbox__factory(signer);
enrolledInbox = await inboxFactory.deploy(
localDomain,
processGas,
reserveGas,
);
enrolledInbox = await inboxFactory.deploy(localDomain);
// The ValidatorManager is unused in these tests *but* needs to be a
// contract.
await enrolledInbox.initialize(
@ -79,11 +73,7 @@ describe('XAppConnectionManager', async () => {
it('Owner can enroll a inbox', async () => {
const newRemoteDomain = 3000;
const inboxFactory = new TestInbox__factory(signer);
const newInbox = await inboxFactory.deploy(
localDomain,
processGas,
reserveGas,
);
const newInbox = await inboxFactory.deploy(localDomain);
// Assert new inbox not considered inbox before enrolled
expect(await connectionManager.isInbox(newInbox.address)).to.be.false;

@ -102,8 +102,7 @@ describe('GovernanceRouter', async () => {
const inbox = abacus.inbox(localDomain, remoteDomain);
await inbox.setMessageProven(fakeMessage);
// Expect inbox processing to fail when reverting in handle
let success = await inbox.callStatic.testProcess(fakeMessage);
expect(success).to.be.false;
await expect(inbox.testProcess(fakeMessage)).to.be.revertedWith('!router');
});
describe('when not in recovery mode', async () => {

@ -1,8 +1,6 @@
import { CoreConfig } from '../../../src/core';
export const core: CoreConfig = {
processGas: 850_000,
reserveGas: 15_000,
validators: {
alfajores: '0x91631845fab02614e53e5F5A68dFBB0E2f1a9B6d',
fuji: '0x91631845fab02614e53e5F5A68dFBB0E2f1a9B6d',

@ -1,8 +1,6 @@
import { CoreConfig } from '../../../src/core';
export const core: CoreConfig = {
processGas: 850_000,
reserveGas: 15_000,
validators: {
// Hardhat accounts 1-4
alfajores: '0x70997970c51812dc3a010c7d01b50e0d17dc79c8',

@ -1,8 +1,6 @@
import { CoreConfig } from '../../../src/core';
export const core: CoreConfig = {
processGas: 850_000,
reserveGas: 15_000,
validators: {
avalanche: '0x6e29236E86a039F8225834F7E7cd4122dc166e51',
celo: '0x703643995262c92ab013E3CCA810BdcB9239d45a',

@ -1,8 +1,6 @@
import { CoreConfig } from '../../../src/core';
export const core: CoreConfig = {
processGas: 850_000,
reserveGas: 15_000,
validators: {
alfajores: '0x201dd86063Dc251cA5a576d1b7365C38e5fB4CD5',
kovan: '0x201dd86063Dc251cA5a576d1b7365C38e5fB4CD5',

@ -1,8 +1,6 @@
import { CoreConfig } from '../../../src/core';
export const core: CoreConfig = {
processGas: 850_000,
reserveGas: 15_000,
validators: {
alfajores: '0xf0B3C01E16cE288f7Cd7112B4b2F5A859Ba72307',
gorli: '0xDd89dCA09Ef81154dAf919b4d7C33f9d8DCf6c7C',

@ -1,16 +1,10 @@
import {
getCoreDeploy,
getCoreDirectory,
getCoreConfig,
getEnvironment,
} from './utils';
import { getCoreDeploy, getCoreDirectory, getEnvironment } from './utils';
import { ImplementationDeployer } from '../src/core/implementation';
async function main() {
const environment = await getEnvironment();
const coreDeploy = await getCoreDeploy(environment);
const coreConfig = await getCoreConfig(environment);
const deployer = new ImplementationDeployer(coreDeploy, coreConfig);
const deployer = new ImplementationDeployer(coreDeploy);
await deployer.deployInboxImplementations();
coreDeploy.writeOutput(getCoreDirectory(environment));
}

@ -1,4 +1,3 @@
import { BigNumberish } from 'ethers';
import { CoreConfigAddresses } from './addresses';
import { ChainName } from './chain';
import { DeployEnvironment } from '../config';
@ -6,7 +5,5 @@ import { DeployEnvironment } from '../config';
export interface CoreConfig {
environment: DeployEnvironment;
recoveryTimelock: number;
processGas: BigNumberish;
reserveGas: BigNumberish;
addresses: Partial<Record<ChainName, CoreConfigAddresses>>;
}

@ -73,7 +73,7 @@ export class CoreInstance extends CommonInstance<CoreContracts> {
chain,
new Inbox__factory(chain.signer),
upgradeBeaconController.address,
[domain, config.processGas, config.reserveGas],
[domain],
initArgs,
);
} else {

@ -3,15 +3,12 @@ import { types } from '@abacus-network/utils';
import { CoreDeploy } from './CoreDeploy';
import { CoreInstance } from './CoreInstance';
import { CoreContracts } from './CoreContracts';
import { CoreConfig } from './types';
export class ImplementationDeployer {
private deploy: CoreDeploy;
private config: CoreConfig;
constructor(deploy: CoreDeploy, config: CoreConfig) {
constructor(deploy: CoreDeploy) {
this.deploy = deploy;
this.config = config;
}
deployOutboxImplementations(): Promise<void> {
@ -53,8 +50,6 @@ export class ImplementationDeployer {
const factory = new Inbox__factory(signer);
const implementation = await factory.deploy(
domain,
this.config.processGas,
this.config.reserveGas,
this.deploy.chains[domain].overrides,
);
const addresses = this.deploy.instances[domain].contracts.toObject();

@ -1,4 +1,3 @@
import { BigNumberish } from 'ethers';
import { types } from '@abacus-network/utils';
import { ProxiedAddress } from '../common';
@ -11,7 +10,5 @@ export type CoreContractAddresses = {
};
export type CoreConfig = {
processGas: BigNumberish;
reserveGas: BigNumberish;
validators: Record<string, types.Address>;
};

@ -30,8 +30,6 @@ const testPolygon: ChainConfigWithoutSigner = {
};
export const testCore: CoreConfig = {
processGas: 850_000,
reserveGas: 15_000,
validators: {
celo: '0x91631845fab02614e53e5F5A68dFBB0E2f1a9B6d',
polygon: '0x91631845fab02614e53e5F5A68dFBB0E2f1a9B6d',

@ -27,9 +27,6 @@ export type TestAbacusInstance = {
inboxes: Record<types.Domain, TestInbox>;
};
const PROCESS_GAS = 850_000;
const RESERVE_GAS = 15_000;
export class TestAbacusDeploy extends TestDeploy<
TestAbacusInstance,
TestAbacusConfig
@ -81,7 +78,7 @@ export class TestAbacusDeploy extends TestDeploy<
// this.remotes reads this.instances which has not yet been set.
const remotes = Object.keys(this.config.signer).map((d) => parseInt(d));
const deploys = remotes.map(async (remote) => {
const inbox = await inboxFactory.deploy(domain, PROCESS_GAS, RESERVE_GAS);
const inbox = await inboxFactory.deploy(domain);
await inbox.initialize(
remote,
validatorManager.address,

Loading…
Cancel
Save