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

Fix: ToB-22 (using msg.value in a loop) #1453

Merged
merged 3 commits into from
Oct 23, 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
20 changes: 15 additions & 5 deletions packages/contracts-core/contracts/client/BaseClient.sol
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,25 @@
* This function is not exposed in BaseClient, as the message encoding is implemented by the child contract.
* > Will revert if the trusted sender is not set for the destination domain.
* @param destination_ Domain of the destination chain
* @param tipsValue Tips to be paid for sending the message
* @param request Encoded message execution request on destination chain
* @param content The message content
*/
function _sendBaseMessage(uint32 destination_, MessageRequest memory request, bytes memory content)
internal
returns (uint32 messageNonce, bytes32 messageHash)
{
function _sendBaseMessage(
uint32 destination_,
uint256 tipsValue,
MessageRequest memory request,
bytes memory content
) internal returns (uint32 messageNonce, bytes32 messageHash) {
// Send message to the trusted sender on destination chain with the defined optimistic period.
// Note: this will revert if the trusted sender is not set for the destination domain.
return _sendBaseMessage(destination_, trustedSender(destination_), optimisticPeriod(), request, content);
return _sendBaseMessage({
destination_: destination_,
recipient: trustedSender(destination_),
optimisticPeriod: optimisticPeriod(),
tipsValue: tipsValue,
request: request,
content: content
});
}

Check warning

Code scanning / Slither

Dead-code Warning

}
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,20 @@ abstract contract MessageRecipient is IMessageRecipient {
* @param destination_ Domain of the destination chain
* @param recipient Address of the recipient on destination chain
* @param optimisticPeriod Optimistic period for the message
* @param tipsValue Tips to be paid for sending the message
* @param request Message execution request on destination chain
* @param content The message content
*/
function _sendBaseMessage(
uint32 destination_,
bytes32 recipient,
uint32 optimisticPeriod,
uint256 tipsValue,
MessageRequest memory request,
bytes memory content
) internal returns (uint32 messageNonce, bytes32 messageHash) {
if (recipient == 0) revert IncorrectRecipient();
return InterfaceOrigin(origin).sendBaseMessage{value: msg.value}(
return InterfaceOrigin(origin).sendBaseMessage{value: tipsValue}(
destination_, recipient, optimisticPeriod, _encodeRequest(request), content
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,14 @@ contract PingPongClient is MessageRecipient {
// TODO: this probably shouldn't be hardcoded
MessageRequest memory request = MessageRequest({gasDrop: 0, gasLimit: 500_000, version: 0});
bytes memory content = abi.encode(message);
_sendBaseMessage(destination_, recipient, _optimisticPeriod(), request, content);
_sendBaseMessage({
destination_: destination_,
recipient: recipient,
optimisticPeriod: _optimisticPeriod(),
tipsValue: 0,
request: request,
content: content
});
}

/// @dev Initiate a new Ping-Pong round.
Expand Down
11 changes: 9 additions & 2 deletions packages/contracts-core/contracts/client/TestClient.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,21 @@ contract TestClient is MessageRecipient {
function sendMessage(
uint32 destination_,
address recipientAddress,
uint32 optimisticSeconds,
uint32 optimisticPeriod,
uint64 gasLimit,
uint32 version,
bytes memory content
) external payable {
bytes32 recipient = TypeCasts.addressToBytes32(recipientAddress);
MessageRequest memory request = MessageRequest({gasDrop: 0, gasLimit: gasLimit, version: version});
(uint32 nonce,) = _sendBaseMessage(destination_, recipient, optimisticSeconds, request, content);
(uint32 nonce,) = _sendBaseMessage({
destination_: destination_,
recipient: recipient,
optimisticPeriod: optimisticPeriod,
tipsValue: msg.value,
request: request,
content: content
});
emit MessageSent(destination_, nonce, TypeCasts.addressToBytes32(address(this)), recipient, content);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ contract BaseClientHarness is BaseClient, BaseClientHarnessEvents {
constructor(address origin_, address destination_) BaseClient(origin_, destination_) {}

/// @notice Exposes _sendBaseMessage for testing
function sendBaseMessage(uint32 destination_, MessageRequest memory request, bytes memory content)
external
payable
returns (uint32 messageNonce, bytes32 messageHash)
{
return _sendBaseMessage(destination_, request, content);
function sendBaseMessage(
uint32 destination_,
uint256 tipsValue,
MessageRequest memory request,
bytes memory content
) external payable returns (uint32 messageNonce, bytes32 messageHash) {
return _sendBaseMessage({destination_: destination_, tipsValue: tipsValue, request: request, content: content});
}

/// @notice Exposes _getMinimumTipsValue for testing
Expand Down
20 changes: 18 additions & 2 deletions packages/contracts-core/test/suite/client/BaseClient.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ contract BaseClientTest is SynapseTest {
);
vm.expectCall(origin, tipsValue, expectedCall);
vm.prank(user);
(uint32 nonce_, bytes32 msgHash_) = client.sendBaseMessage{value: tipsValue}(destination_, request, content);
(uint32 nonce_, bytes32 msgHash_) =
client.sendBaseMessage{value: tipsValue}(destination_, tipsValue, request, content);
assertEq(nonce_, nonce);
assertEq(msgHash_, msgHash);
}
Expand All @@ -77,7 +78,22 @@ contract BaseClientTest is SynapseTest {
vm.deal(user, tipsValue);
vm.expectRevert(IncorrectRecipient.selector);
vm.prank(user);
client.sendBaseMessage{value: tipsValue}(destination_, request, "");
client.sendBaseMessage{value: tipsValue}(destination_, tipsValue, request, "");
}

function test_sendBaseMessage_tipsValueNotMsgValue() public {
address user = makeAddr("User");
uint32 destination_ = 1;
uint256 tipsValue = 1337;
uint256 msgValue = 6969;
// Use empty request for this test
MessageRecipient.MessageRequest memory request;
vm.deal(user, msgValue);
bytes memory expectedCall = abi.encodeWithSelector(InterfaceOrigin.sendBaseMessage.selector);
// Should call origin with provided tipsValue as msg.value
vm.expectCall(origin, tipsValue, expectedCall);
vm.prank(user);
client.sendBaseMessage{value: msgValue}(destination_, tipsValue, request, "");
}

function test_getMinimumTipsValue(
Expand Down
Loading