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(protocol): fix issues in AssignmentHook #15486

Merged
merged 12 commits into from
Jan 13, 2024
4 changes: 4 additions & 0 deletions packages/protocol/contracts/L1/ITaikoL1.sol
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,8 @@ interface ITaikoL1 {
/// @notice Verifies up to a certain number of blocks.
/// @param maxBlocksToVerify Max number of blocks to verify.
function verifyBlocks(uint64 maxBlocksToVerify) external;

/// @notice Gets the configuration of the TaikoL1 contract.
/// @return Config struct containing configuration parameters.
function getConfig() external view returns (TaikoData.Config memory);
}
5 changes: 2 additions & 3 deletions packages/protocol/contracts/L1/TaikoL1.sol
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,8 @@ contract TaikoL1 is
return ITierProvider(resolve("tier_provider", false)).getMinTier(rand);
}

/// @notice Gets the configuration of the TaikoL1 contract.
/// @return Config struct containing configuration parameters.
function getConfig() public view virtual returns (TaikoData.Config memory) {
/// @inheritdoc ITaikoL1
function getConfig() public view virtual override returns (TaikoData.Config memory) {
// All hard-coded configurations:
// - treasury: the actual TaikoL2 address.
// - blockMaxTxs: 150 (limited by the PSE zkEVM circuits)
Expand Down
35 changes: 24 additions & 11 deletions packages/protocol/contracts/L1/hooks/AssignmentHook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
import "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
import "../../common/EssentialContract.sol";
import "../../libs/LibAddress.sol";
import "../TaikoData.sol";
import "../ITaikoL1.sol";
import "./IHook.sol";

/// @title AssignmentHook
Expand All @@ -39,7 +39,7 @@ contract AssignmentHook is EssentialContract, IHook {

struct Input {
ProverAssignment assignment;
uint256 tip;
uint256 tip; // A tip to L1 block builder
}

// Max gas paying the prover. This should be large enough to prevent the
Expand Down Expand Up @@ -70,6 +70,11 @@ contract AssignmentHook is EssentialContract, IHook {
nonReentrant
onlyFromNamed("taiko")
{
// Note that
// - 'msg.sender' is the TaikoL1 contract address
// - 'block.coinbase' is the L1 block builder
// - 'meta.coinbase' is the L2 block proposer

Input memory input = abi.decode(data, (Input));
ProverAssignment memory assignment = input.assignment;

Expand All @@ -85,15 +90,16 @@ contract AssignmentHook is EssentialContract, IHook {

// Hash the assignment with the blobHash, this hash will be signed by
// the prover, therefore, we add a string as a prefix.
bytes32 hash = hashAssignment(assignment, msg.sender, meta.blobHash);
address taikoL1Address = msg.sender;
bytes32 hash = hashAssignment(assignment, taikoL1Address, meta.blobHash);

if (!blk.assignedProver.isValidSignature(hash, assignment.signature)) {
revert HOOK_ASSIGNMENT_INVALID_SIG();
}

// Send the liveness bond to the Taiko contract
IERC20 tko = IERC20(resolve("taiko_token", false));
tko.transferFrom(blk.assignedProver, msg.sender, blk.livenessBond);
tko.transferFrom(blk.assignedProver, taikoL1Address, blk.livenessBond);

// Find the prover fee using the minimal tier
uint256 proverFee = _getProverFee(assignment.tierFees, meta.minTier);
Expand All @@ -102,12 +108,13 @@ contract AssignmentHook is EssentialContract, IHook {
// Ether or ERC20 tokens.
uint256 refund;
if (assignment.feeToken == address(0)) {
if (msg.value < proverFee + input.tip) {
uint256 totalFee = proverFee + input.tip;
if (msg.value < totalFee) {
revert HOOK_ASSIGNMENT_INSUFFICIENT_FEE();
}

unchecked {
refund = msg.value - proverFee - input.tip;
refund = msg.value - totalFee;
}

// Paying Ether
Expand All @@ -120,7 +127,9 @@ contract AssignmentHook is EssentialContract, IHook {
refund = msg.value - input.tip;
}
// Paying ERC20 tokens
IERC20(assignment.feeToken).safeTransferFrom(msg.sender, blk.assignedProver, proverFee);
IERC20(assignment.feeToken).safeTransferFrom(
dantaik marked this conversation as resolved.
Show resolved Hide resolved
meta.coinbase, blk.assignedProver, proverFee
davidtaikocha marked this conversation as resolved.
Show resolved Hide resolved
);
}

// block.coinbase can be address(0) in tests
Expand All @@ -129,25 +138,29 @@ contract AssignmentHook is EssentialContract, IHook {
}

if (refund != 0) {
msg.sender.sendEther(refund);
// Send all remaininger Ether back to TaikoL1 contract
taikoL1Address.sendEther(refund);
}

emit BlockAssigned(blk.assignedProver, meta, assignment);
}

function hashAssignment(
ProverAssignment memory assignment,
address taikoAddress,
address taikoL1Address,
bytes32 blobHash
)
public
pure
view
returns (bytes32)
{
return keccak256(
abi.encode(
"PROVER_ASSIGNMENT",
taikoAddress,
ITaikoL1(taikoL1Address).getConfig().chainId,
davidtaikocha marked this conversation as resolved.
Show resolved Hide resolved
taikoL1Address,
address(this),
assignment.metaHash,
blobHash,
assignment.feeToken,
assignment.expiry,
Expand Down
6 changes: 4 additions & 2 deletions packages/protocol/contracts/L1/libs/LibProposing.sol
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,10 @@ library LibProposing {
}

// Check that after hooks, the Taiko Token balance of this contract
// have increased by at least config.livenessBond
if (tko.balanceOf(address(this)) < tkoBalance + config.livenessBond) {
// have increased by the same amount as config.livenessBond (to prevent)
// multiple draining payments by a malicious proposer nesting the same
// hook.
if (tko.balanceOf(address(this)) != tkoBalance + config.livenessBond) {
dantaik marked this conversation as resolved.
Show resolved Hide resolved
dantaik marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check TKO balance change strict using == rather than >=.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adaki2004 with this change, maybe we don't have to check "==" here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still recommend having a strict equality, for two reasons
a) I don't see a benefit for transferring more than the liveness bond, it would increase the cost to the prover and the extra funds would be stuck in the TaikoL1 contract.
b) If multiple hooks are available which can transfer TKO to the TaikoL1 contract the proposer could attempt to use both e.g. If there is a third party hook, a proposer could attempt to use both hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still recommend having a strict equality, for two reasons

I think this is a strict equalitsy, no ? (if NOT -> revert)
if (tko.balanceOf(address(this)) != tkoBalance + config.livenessBond) revert L1_LIVENESS_BOND_NOT_RECEIVED

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep this PR currently has a strict equality, which I currently see as a nicer solution. I was responding to @dantaik comment about potentially moving back to inequality.

@adaki2004 with this change, maybe we don't have to check "==" here?

revert L1_LIVENESS_BOND_NOT_RECEIVED();
}
}
Expand Down
Loading