Skip to content

Commit

Permalink
inbox fee cast safely, fix deadline bug, address nits (#604)
Browse files Browse the repository at this point in the history 
* inbox fee cast safely, address nits

* var names with _ and consistent fn name

* remove helper_computeentrykey
  • Loading branch information
@rahul-kothari
rahul-kothari authored May 17, 2023
1 parent 83a0f63 commit e61b65f
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 111 deletions.
5 changes: 1 addition & 4 deletions l1-contracts/src/core/interfaces/messagebridge/IInbox.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@ interface IInbox {
event L1ToL2MessageCancelled(bytes32 indexed entryKey);

/// @notice Given a message, computes an entry key for the Inbox
function computeMessageKey(DataStructures.L1ToL2Msg memory message)
external
pure
returns (bytes32);
function computeEntryKey(DataStructures.L1ToL2Msg memory message) external pure returns (bytes32);

/**
* @notice Inserts an entry into the Inbox
Expand Down
41 changes: 23 additions & 18 deletions l1-contracts/src/core/messagebridge/Inbox.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {MessageBox} from "./MessageBox.sol";
*/
contract Inbox is MessageBox, IInbox {
error Inbox__DeadlineBeforeNow();
error Inbox__FeeTooHigh();
error Inbox__NotPastDeadline();
error Inbox__PastDeadline();
error Inbox__Unauthorized();
Expand All @@ -24,20 +25,20 @@ contract Inbox is MessageBox, IInbox {

/**
* @notice Given a message, computes an entry key for the Inbox
* @param message - The L1 to L2 message
* @param _message - The L1 to L2 message
* @return The hash of the message (used as the key of the entry in the set)
*/
function computeMessageKey(DataStructures.L1ToL2Msg memory message) public pure returns (bytes32) {
function computeEntryKey(DataStructures.L1ToL2Msg memory _message) public pure returns (bytes32) {
return bytes32(
uint256(
sha256(
abi.encode(
message.sender,
message.recipient,
message.content,
message.secretHash,
message.deadline,
message.fee
_message.sender,
_message.recipient,
_message.content,
_message.secretHash,
_message.deadline,
_message.fee
)
)
) % P // TODO: Replace mod P later on when we have a better idea of how to handle Fields.
Expand All @@ -50,6 +51,7 @@ contract Inbox is MessageBox, IInbox {
* @dev msg.value - The fee provided to sequencer for including the entry
* @param _recipient - The recipient of the entry
* @param _deadline - The deadline to consume a message. Only after it, can a message be cancalled.
* it is uint32 to for slot packing of the Entry struct. Should work until Feb 2106.
* @param _content - The content of the entry (application specific)
* @param _secretHash - The secret hash of the entry (make it possible to hide when a specific entry is consumed on L2)
* @return The key of the entry in the set
Expand All @@ -61,6 +63,9 @@ contract Inbox is MessageBox, IInbox {
bytes32 _secretHash
) external payable returns (bytes32) {
if (_deadline <= block.timestamp) revert Inbox__DeadlineBeforeNow();
// `fee` is uint64 for slot packing of the Entry struct. uint64 caps at ~18.4 ETH which should be enough.
// we revert here to safely cast msg.value into uint64.
if (msg.value > type(uint64).max) revert Inbox__FeeTooHigh();
uint64 fee = uint64(msg.value);
DataStructures.L1ToL2Msg memory message = DataStructures.L1ToL2Msg({
sender: DataStructures.L1Actor(msg.sender, block.chainid),
Expand All @@ -71,7 +76,7 @@ contract Inbox is MessageBox, IInbox {
fee: fee
});

bytes32 key = computeMessageKey(message);
bytes32 key = computeEntryKey(message);
_insert(key, fee, _deadline);

emit MessageAdded(
Expand Down Expand Up @@ -102,8 +107,8 @@ contract Inbox is MessageBox, IInbox {
returns (bytes32 entryKey)
{
if (msg.sender != _message.sender.actor) revert Inbox__Unauthorized();
if (_message.deadline <= block.timestamp) revert Inbox__NotPastDeadline();
entryKey = computeMessageKey(_message);
if (block.timestamp <= _message.deadline) revert Inbox__NotPastDeadline();
entryKey = computeEntryKey(_message);
_consume(entryKey);
feesAccrued[_feeCollector] += _message.fee;
emit L1ToL2MessageCancelled(entryKey);
Expand All @@ -113,16 +118,16 @@ contract Inbox is MessageBox, IInbox {
* @notice Batch consumes entries from the Inbox
* @dev Only callable by the rollup contract
* @dev Will revert if the message is already past deadline
* @param entryKeys - Array of entry keys (hash of the messages)
* @param _entryKeys - Array of entry keys (hash of the messages)
* @param _feeCollector - The address to receive the "fee"
*/
function batchConsume(bytes32[] memory entryKeys, address _feeCollector) external onlyRollup {
function batchConsume(bytes32[] memory _entryKeys, address _feeCollector) external onlyRollup {
uint256 totalFee = 0;
for (uint256 i = 0; i < entryKeys.length; i++) {
// TODO: Combine these to optimise for gas.
DataStructures.Entry memory entry = get(entryKeys[i]);
if (entry.deadline > block.timestamp) revert Inbox__PastDeadline();
_consume(entryKeys[i]);
for (uint256 i = 0; i < _entryKeys.length; i++) {
DataStructures.Entry memory entry = get(_entryKeys[i]);
// cant consume if we are already past deadline.
if (block.timestamp > entry.deadline) revert Inbox__PastDeadline();
_consume(_entryKeys[i]);
totalFee += entry.fee;
}
feesAccrued[_feeCollector] += totalFee;
Expand Down
1 change: 0 additions & 1 deletion l1-contracts/src/core/messagebridge/MessageBox.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ abstract contract MessageBox is IMessageBox {
uint32 deadlinePassed
);
error MessageBox__NothingToConsume(bytes32 entryKey);
error MessageBox__OversizedContent();

// Prime field order
uint256 internal constant P =
Expand Down
122 changes: 60 additions & 62 deletions l1-contracts/test/Inbox.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,27 +33,6 @@ contract InboxTest is Test {
registry.setAddresses(rollup, address(inbox), address(0x0));
}

function _helper_computeEntryKey(DataStructures.L1ToL2Msg memory message)
internal
pure
returns (bytes32)
{
return bytes32(
uint256(
sha256(
abi.encode(
message.sender,
message.recipient,
message.content,
message.secretHash,
message.deadline,
message.fee
)
)
) % 21888242871839275222246405745257275088548364400416034343698204186575808495617
);
}

function _fakeMessage() internal view returns (DataStructures.L1ToL2Msg memory) {
return DataStructures.L1ToL2Msg({
sender: DataStructures.L1Actor({actor: address(this), chainId: block.chainid}),
Expand All @@ -68,33 +47,34 @@ contract InboxTest is Test {
});
}

function testFuzzSendL2Msg(DataStructures.L1ToL2Msg memory message) public {
function testFuzzSendL2Msg(DataStructures.L1ToL2Msg memory _message) public {
// fix message.sender and deadline:
message.sender = DataStructures.L1Actor({actor: address(this), chainId: block.chainid});
if (message.deadline <= block.timestamp) {
message.deadline = uint32(block.timestamp + 100);
_message.sender = DataStructures.L1Actor({actor: address(this), chainId: block.chainid});
if (_message.deadline <= block.timestamp) {
_message.deadline = uint32(block.timestamp + 100);
}
bytes32 expectedEntryKey = _helper_computeEntryKey(message);
bytes32 expectedEntryKey = inbox.computeEntryKey(_message);
vm.expectEmit(true, true, true, true);
// event we expect
emit MessageAdded(
expectedEntryKey,
message.sender.actor,
message.recipient.actor,
message.sender.chainId,
message.recipient.version,
message.deadline,
message.fee,
message.content
_message.sender.actor,
_message.recipient.actor,
_message.sender.chainId,
_message.recipient.version,
_message.deadline,
_message.fee,
_message.content
);
// event we will get
bytes32 entryKey = inbox.sendL2Message{value: message.fee}(
message.recipient, message.deadline, message.content, message.secretHash
bytes32 entryKey = inbox.sendL2Message{value: _message.fee}(
_message.recipient, _message.deadline, _message.content, _message.secretHash
);
assertEq(entryKey, expectedEntryKey);
assertEq(inbox.get(entryKey).count, 1);
assertEq(inbox.get(entryKey).fee, message.fee);
assertEq(inbox.get(entryKey).deadline, message.deadline);
DataStructures.Entry memory entry = inbox.get(entryKey);
assertEq(entry.count, 1);
assertEq(entry.fee, _message.fee);
assertEq(entry.deadline, _message.deadline);
}

function testSendMultipleSameL2Messages() public {
Expand Down Expand Up @@ -128,17 +108,19 @@ contract InboxTest is Test {

function testRevertIfCancellingMessageWhenDeadlineHasntPassed() public {
DataStructures.L1ToL2Msg memory message = _fakeMessage();
message.deadline = uint32(block.timestamp + 1000);
inbox.sendL2Message{value: message.fee}(
message.recipient, message.deadline, message.content, message.secretHash
);
skip(1000); // block.timestamp now +1000 ms.
skip(500); // deadline = 1000. block.timestamp = 500. Not cancellable:
vm.expectRevert(Inbox.Inbox__NotPastDeadline.selector);
inbox.cancelL2Message(message, address(0x1));
}

function testRevertIfCancellingNonExistentMessage() public {
DataStructures.L1ToL2Msg memory message = _fakeMessage();
bytes32 entryKey = _helper_computeEntryKey(message);
bytes32 entryKey = inbox.computeEntryKey(message);
skip(500); // make message cancellable.
vm.expectRevert(
abi.encodeWithSelector(MessageBox.MessageBox__NothingToConsume.selector, entryKey)
);
Expand All @@ -151,21 +133,23 @@ contract InboxTest is Test {
bytes32 expectedEntryKey = inbox.sendL2Message{value: message.fee}(
message.recipient, message.deadline, message.content, message.secretHash
);
skip(500); // make message cancellable.

vm.expectEmit(true, false, false, false);
// event we expect
emit L1ToL2MessageCancelled(expectedEntryKey);
// event we will get
inbox.cancelL2Message(message, feeCollector);
// fees accrued as expected:
assertEq(inbox.feesAccrued(feeCollector), message.fee);

// no such message to consume:
bytes32[] memory entryKeys = new bytes32[](1);
entryKeys[0] = expectedEntryKey;
vm.expectRevert(
abi.encodeWithSelector(MessageBox.MessageBox__NothingToConsume.selector, expectedEntryKey)
);
inbox.get(expectedEntryKey);

// fees accrued as expected:
assertEq(inbox.feesAccrued(feeCollector), message.fee);
inbox.batchConsume(entryKeys, feeCollector);
}

function testRevertIfNotConsumingFromRollup() public {
Expand All @@ -179,10 +163,10 @@ contract InboxTest is Test {
function testRevertIfOneKeyIsPastDeadlineWhenBatchConsuming() public {
DataStructures.L1ToL2Msg memory message = _fakeMessage();
bytes32 entryKey1 = inbox.sendL2Message{value: message.fee}(
message.recipient, uint32(block.timestamp + 100), message.content, message.secretHash
message.recipient, uint32(block.timestamp + 200), message.content, message.secretHash
);
bytes32 entryKey2 = inbox.sendL2Message{value: message.fee}(
message.recipient, uint32(block.timestamp + 200), message.content, message.secretHash
message.recipient, uint32(block.timestamp + 100), message.content, message.secretHash
);
bytes32 entryKey3 = inbox.sendL2Message{value: message.fee}(
message.recipient, uint32(block.timestamp + 300), message.content, message.secretHash
Expand All @@ -192,46 +176,60 @@ contract InboxTest is Test {
entryKeys[1] = entryKey2;
entryKeys[2] = entryKey3;

skip(150); // block.timestamp now +150 ms. entryKey2 and entryKey3 is past deadline
skip(150); // block.timestamp now +150 ms. entryKey2 is past deadline
vm.expectRevert(Inbox.Inbox__PastDeadline.selector);
inbox.batchConsume(entryKeys, address(0x1));
}

function testRevertIfConsumingAMessageThatDoesntExist(bytes32[] memory entryKeys) public {
if (entryKeys.length == 0) {
entryKeys = new bytes32[](1);
entryKeys[0] = bytes32("random");
function testFuzzRevertIfConsumingAMessageThatDoesntExist(bytes32[] memory _entryKeys) public {
if (_entryKeys.length == 0) {
_entryKeys = new bytes32[](1);
_entryKeys[0] = bytes32("random");
}
vm.expectRevert(
abi.encodeWithSelector(MessageBox.MessageBox__NothingToConsume.selector, _entryKeys[0])
);
inbox.batchConsume(_entryKeys, address(0x1));
}

function testRevertIfConsumingTheSameMessageMoreThanTheCountOfEntries() public {
DataStructures.L1ToL2Msg memory message = _fakeMessage();
address feeCollector = address(0x1);
bytes32 entryKey = inbox.sendL2Message{value: message.fee}(
message.recipient, message.deadline, message.content, message.secretHash
);
bytes32[] memory entryKeys = new bytes32[](1);
entryKeys[0] = entryKey;

inbox.batchConsume(entryKeys, feeCollector);
assertEq(inbox.feesAccrued(feeCollector), message.fee);

// consuming this again should fail:
vm.expectRevert(
abi.encodeWithSelector(MessageBox.MessageBox__NothingToConsume.selector, entryKeys[0])
);
inbox.batchConsume(entryKeys, address(0x1));
inbox.batchConsume(entryKeys, feeCollector);
}

function testBatchConsume(DataStructures.L1ToL2Msg[] memory messages) public {
bytes32[] memory entryKeys = new bytes32[](messages.length);
function testFuzzBatchConsume(DataStructures.L1ToL2Msg[] memory _messages) public {
bytes32[] memory entryKeys = new bytes32[](_messages.length);
uint256 expectedTotalFee = 0;
address feeCollector = address(0x1);
uint256 maxDeadline = 0; // for skipping time (to avoid past deadline revert)

// insert messages:
for (uint256 i = 0; i < messages.length; i++) {
DataStructures.L1ToL2Msg memory message = messages[i];
// fix message.sender and deadline:
for (uint256 i = 0; i < _messages.length; i++) {
DataStructures.L1ToL2Msg memory message = _messages[i];
// fix message.sender and deadline to be more than current time:
message.sender = DataStructures.L1Actor({actor: address(this), chainId: block.chainid});
if (message.deadline <= block.timestamp) {
message.deadline = uint32(block.timestamp + 100);
}
if (message.deadline > maxDeadline) {
maxDeadline = message.deadline;
}
expectedTotalFee += message.fee;
entryKeys[i] = inbox.sendL2Message{value: message.fee}(
message.recipient, message.deadline, message.content, message.secretHash
);
}

skip(maxDeadline + 100);
// batch consume:
inbox.batchConsume(entryKeys, feeCollector);

Expand Down
Loading

0 comments on commit e61b65f

Please sign in to comment.