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(protocol): check msg.value in DelegateOwner #16930

Closed
wants to merge 4 commits into from
Closed
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
28 changes: 15 additions & 13 deletions packages/protocol/contracts/L2/DelegateOwner.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ import "../bridge/IBridge.sol";
/// @custom:security-contact [email protected]
contract DelegateOwner is EssentialContract, IMessageInvocable {
/// @notice The owner chain ID.
uint64 public l1ChainId;
uint64 public srcChainId;

/// @notice The next transaction ID.
uint64 public nextTxId;

/// @notice The real owner on L1, supposedly the DAO.
address public realOwner;
address public srcOwner;

uint256[48] private __gap;

Expand All @@ -34,32 +34,33 @@ contract DelegateOwner is EssentialContract, IMessageInvocable {

error DO_INVALID_PARAM();
error DO_INVALID_TX_ID();
error DO_INVALID_VALUE();
error DO_PERMISSION_DENIED();
error DO_TX_REVERTED();
error DO_UNSUPPORTED();

/// @notice Initializes the contract.
/// @param _realOwner The real owner on L1 that can send a cross-chain message to invoke
/// @param _srcOwner The real owner on L1 that can send a cross-chain message to invoke
/// `onMessageInvocation`.
/// @param _addressManager The address of the {AddressManager} contract.
/// @param _l1ChainId The L1 chain's ID.
/// @param _srcChainId The L1 chain's ID.
function init(
address _realOwner,
address _srcOwner,
address _addressManager,
uint64 _l1ChainId
uint64 _srcChainId
)
external
initializer
{
// This contract's owner will be itself.
__Essential_init(address(this), _addressManager);

if (_realOwner == address(0) || _l1ChainId == 0 || _l1ChainId == block.chainid) {
if (_srcOwner == address(0) || _srcChainId == 0 || _srcChainId == block.chainid) {
revert DO_INVALID_PARAM();
}

realOwner = _realOwner;
l1ChainId = _l1ChainId;
srcOwner = _srcOwner;
srcChainId = _srcChainId;
}

/// @inheritdoc IMessageInvocable
Expand All @@ -70,19 +71,20 @@ contract DelegateOwner is EssentialContract, IMessageInvocable {
payable
onlyFromNamed(LibStrings.B_BRIDGE)
{
(uint64 txId, address target, bytes memory txdata) =
abi.decode(_data, (uint64, address, bytes));
(uint64 txId, address target, uint256 txValue, bytes memory txdata) =
abi.decode(_data, (uint64, address, uint256, bytes));

if (txId != nextTxId) revert DO_INVALID_TX_ID();
if (txValue != msg.value) revert DO_INVALID_VALUE();

IBridge.Context memory ctx = IBridge(msg.sender).context();
if (ctx.srcChainId != l1ChainId || ctx.from != realOwner) {
if (ctx.srcChainId != srcChainId || ctx.from != srcOwner) {
revert DO_PERMISSION_DENIED();
}
nextTxId++;
// Sending ether along with the function call. Although this is sending Ether from this
// contract back to itself, txData's function can now be payable.
(bool success,) = target.call{ value: msg.value }(txdata);
(bool success,) = target.call{ value: txValue }(txdata);
if (!success) revert DO_TX_REVERTED();

emit TransactionExecuted(txId, target, bytes4(txdata));
Expand Down
26 changes: 23 additions & 3 deletions packages/protocol/test/bridge/Bridge.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ contract BridgeTest is TaikoTest {
address(mockDAO),
abi.encodeCall(
DelegateOwner.onMessageInvocation,
abi.encode(0, address(destChainBridge), pauseCall)
abi.encode(0, address(destChainBridge), 0, pauseCall)
)
);

Expand All @@ -279,6 +279,7 @@ contract BridgeTest is TaikoTest {
}

function test_Bridge_authorize_signal_service_via_delegate_owner() public {
uint256 chainId = block.chainid;
assertEq(mockProofSignalService.isAuthorized(Alice), false);

bytes memory authorizeCall = abi.encodeCall(SignalService.authorize, (Alice, true));
Expand All @@ -287,7 +288,7 @@ contract BridgeTest is TaikoTest {
address(mockDAO),
abi.encodeCall(
DelegateOwner.onMessageInvocation,
abi.encode(0, address(mockProofSignalService), authorizeCall)
abi.encode(0, address(mockProofSignalService), 0, authorizeCall)
)
);

Expand All @@ -307,6 +308,25 @@ contract BridgeTest is TaikoTest {
assertEq(status == IBridge.Status.DONE, true);

assertEq(mockProofSignalService.isAuthorized(Alice), true);

//----
vm.chainId(chainId);
assertEq(mockProofSignalService.isAuthorized(Carol), false);
authorizeCall = abi.encodeCall(SignalService.authorize, (Carol, true));

message = getDelegateOwnerMessage(
address(mockDAO),
abi.encodeCall(
DelegateOwner.onMessageInvocation,
abi.encode(1, address(mockProofSignalService), 0, authorizeCall)
)
);

vm.chainId(destChainId);
vm.prank(Bob, Bob);
destChainBridge.processMessage(message, proof);

assertEq(mockProofSignalService.isAuthorized(Carol), true);
}

function test_Bridge_upgrade_delegate_owner() public {
Expand All @@ -318,7 +338,7 @@ contract BridgeTest is TaikoTest {
address(mockDAO),
abi.encodeCall(
DelegateOwner.onMessageInvocation,
abi.encode(0, address(delegateOwner), upgradeCall)
abi.encode(0, address(delegateOwner), 0, upgradeCall)
)
);

Expand Down