Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: limit permissionless generic call gas usage #200

Merged
merged 2 commits into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion contracts/handlers/PermissionlessGenericHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import "../interfaces/IHandler.sol";
@notice This contract is intended to be used with the Bridge contract.
*/
contract PermissionlessGenericHandler is IHandler {
uint256 public constant MAX_FEE = 1000000;

address public immutable _bridgeAddress;

modifier onlyBridge() {
Expand Down Expand Up @@ -87,16 +89,19 @@ contract PermissionlessGenericHandler is IHandler {
function deposit(bytes32 resourceID, address depositor, bytes calldata data) external view returns (bytes memory) {
require(data.length >= 76, "Incorrect data length"); // 32 + 2 + 1 + 1 + 20 + 20

uint256 maxFee;
uint16 lenExecuteFuncSignature;
uint8 lenExecuteContractAddress;
uint8 lenExecutionDataDepositor;
address executionDataDepositor;

maxFee = uint256(bytes32(data[:32]));
lenExecuteFuncSignature = uint16(bytes2(data[32:34]));
lenExecuteContractAddress = uint8(bytes1(data[34 + lenExecuteFuncSignature:35 + lenExecuteFuncSignature]));
lenExecutionDataDepositor = uint8(bytes1(data[35 + lenExecuteFuncSignature + lenExecuteContractAddress:36 + lenExecuteFuncSignature + lenExecuteContractAddress]));
executionDataDepositor = address(uint160(bytes20(data[36 + lenExecuteFuncSignature + lenExecuteContractAddress:36 + lenExecuteFuncSignature + lenExecuteContractAddress + lenExecutionDataDepositor])));

require(maxFee < MAX_FEE, 'requested fee too large');
require(depositor == executionDataDepositor, 'incorrect depositor in deposit data');
}

Expand Down Expand Up @@ -143,6 +148,7 @@ contract PermissionlessGenericHandler is IHandler {
executeFuncSignature(address executionDataDepositor, uint[] uintArray, address addr)
*/
function executeProposal(bytes32 resourceID, bytes calldata data) external onlyBridge returns (bytes memory) {
uint256 maxFee;
uint16 lenExecuteFuncSignature;
bytes4 executeFuncSignature;
uint8 lenExecuteContractAddress;
Expand All @@ -151,6 +157,7 @@ contract PermissionlessGenericHandler is IHandler {
address executionDataDepositor;
bytes memory executionData;

maxFee = uint256(bytes32(data[0:32]));
lenExecuteFuncSignature = uint16(bytes2(data[32:34]));
executeFuncSignature = bytes4(data[34:34 + lenExecuteFuncSignature]);
lenExecuteContractAddress = uint8(bytes1(data[34 + lenExecuteFuncSignature:35 + lenExecuteFuncSignature]));
Expand All @@ -160,7 +167,7 @@ contract PermissionlessGenericHandler is IHandler {
executionData = bytes(data[36 + lenExecuteFuncSignature + lenExecuteContractAddress + lenExecutionDataDepositor:]);

bytes memory callData = abi.encodePacked(executeFuncSignature, abi.encode(executionDataDepositor), executionData);
(bool success, bytes memory returndata) = executeContractAddress.call(callData);
(bool success, bytes memory returndata) = executeContractAddress.call{gas: maxFee}(callData);
return abi.encode(success, returndata);
}
}
2 changes: 1 addition & 1 deletion test/handlers/fee/dynamic/calculateFeeGenericEVM.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ contract("DynamicGenericFeeHandlerEVM - [calculateFee]", async (accounts) => {
const sender = accounts[0];
const depositorAddress = accounts[1];
const emptySetResourceData = "0x";
const destinationMaxFee = 2000000;
const destinationMaxFee = 900000;
const msgGasLimit = 2300000;
const ter = 0; // Not used
const feeDataAmount = 0; // Not used
Expand Down
2 changes: 1 addition & 1 deletion test/handlers/fee/dynamic/collectFeeGenericEVM.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ contract("DynamicGenericFeeHandlerEVM - [collectFee]", async (accounts) => {
const depositorAddress = accounts[1];

const emptySetResourceData = "0x";
const destinationMaxFee = 2000000;
const destinationMaxFee = 900000;
const msgGasLimit = 2300000;
const hashOfTestStore = Ethers.utils.keccak256("0xc0ffee");
const fee = Ethers.utils.parseEther("0.000036777");
Expand Down
25 changes: 24 additions & 1 deletion test/handlers/generic/permissionlessDeposit.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ contract("PermissionlessGenericHandler - [deposit]", async (accounts) => {
const depositorAddress = accounts[1];

const feeData = "0x";
const destinationMaxFee = 2000000;
const destinationMaxFee = 900000;
const hashOfTestStore = Ethers.utils.keccak256("0xc0ffee");
const emptySetResourceData = "0x";

Expand Down Expand Up @@ -152,4 +152,27 @@ contract("PermissionlessGenericHandler - [deposit]", async (accounts) => {
"incorrect depositor in deposit data"
);
});


it("should revert if max fee exceeds 1000000", async () => {
const invalidMaxFee = 1000001;
const invalidDepositData = Helpers.createPermissionlessGenericDepositData(
depositFunctionSignature,
TestStoreInstance.address,
invalidMaxFee ,
depositorAddress,
hashOfTestStore
);

await TruffleAssert.reverts(
BridgeInstance.deposit(
destinationDomainID,
resourceID,
invalidDepositData,
feeData,
{from: depositorAddress}
),
"requested fee too large"
);
});
});
70 changes: 69 additions & 1 deletion test/handlers/generic/permissionlessExecuteProposal.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ contract(
const invalidExecutionContractAddress = accounts[4];

const feeData = "0x";
const destinationMaxFee = 2000000;
const destinationMaxFee = 900000;
const hashOfTestStore = Ethers.utils.keccak256("0xc0ffee");
const handlerResponseLength = 64;
const contractCallReturndata = Ethers.constants.HashZero;
Expand Down Expand Up @@ -226,6 +226,74 @@ contract(
);
});

it("ProposalExecution should be emitted even if gas specified too small", async () => {
const num = 6;
const addresses = [BridgeInstance.address, TestStoreInstance.address];
const message = Ethers.utils.hexlify(Ethers.utils.toUtf8Bytes("message"));
const executionData = Helpers.abiEncode(["uint", "address[]", "bytes"], [num, addresses, message]);

// If the target function accepts (address depositor, bytes executionData)
// then this helper can be used
const preparedExecutionData = await TestDepositInstance.prepareDepositData(executionData);
const depositFunctionSignature = Helpers.getFunctionSignature(
TestDepositInstance,
"executePacked"
);
const tooSmallGas = 500;
const depositData = Helpers.createPermissionlessGenericDepositData(
depositFunctionSignature,
TestDepositInstance.address,
tooSmallGas,
depositorAddress,
preparedExecutionData
);
const proposal = {
originDomainID: originDomainID,
depositNonce: expectedDepositNonce,
data: depositData,
resourceID: resourceID,
};
const proposalSignedData = await Helpers.signTypedProposal(
BridgeInstance.address,
[proposal]
);

// relayer1 executes the proposal
const executeTx = await BridgeInstance.executeProposal(
proposal,
proposalSignedData,
{from: relayer1Address}
);
// check that ProposalExecution event is emitted
TruffleAssert.eventEmitted(executeTx, "ProposalExecution", (event) => {
return (
event.originDomainID.toNumber() === originDomainID &&
event.depositNonce.toNumber() === expectedDepositNonce
);
});

// check that deposit nonce isn't unmarked as used in bitmap
assert.isTrue(
await BridgeInstance.isProposalExecuted(
originDomainID,
expectedDepositNonce
)
);

const internalTx = await TruffleAssert.createTransactionResult(
TestDepositInstance,
executeTx.tx
);
TruffleAssert.eventNotEmitted(internalTx, "TestExecute", (event) => {
return (
event.depositor === depositorAddress &&
event.num.toNumber() === num &&
event.addr === TestStoreInstance.address &&
event.message === message
);
});
});

it("call with packed depositData should be successful", async () => {
const num = 5;
const addresses = [BridgeInstance.address, TestStoreInstance.address];
Expand Down
Loading