Layer 1 Assessment - Story

The removeOperator, redelegate and redelegateOnBehalf do not have the chargeFee modifier. As none of them requires the user sending funds, it allows a malicious user to spam the CL by sending these events, which requires consumption from the CL.

Code Location

https://github.com/piplabs/story/blob/e6d2d51550c3eff3f561f8d1b860888ea2bf8060/contracts/src/protocol/IPTokenStaking.sol#L162

    function removeOperator(
        bytes calldata uncmpPubkey,
        address operator
    ) external verifyUncmpPubkeyWithExpectedAddress(uncmpPubkey, msg.sender) 

https://github.com/piplabs/story/blob/e6d2d51550c3eff3f561f8d1b860888ea2bf8060/contracts/src/protocol/IPTokenStaking.sol#L423

    function redelegate(
        bytes calldata delegatorUncmpPubkey,
        bytes calldata validatorUncmpSrcPubkey,
        bytes calldata validatorUncmpDstPubkey,
        uint256 delegationId,
        uint256 amount
    )
        external
        verifyUncmpPubkeyWithExpectedAddress(delegatorUncmpPubkey, msg.sender)
        verifyUncmpPubkey(validatorUncmpSrcPubkey)
        verifyUncmpPubkey(validatorUncmpDstPubkey)

https://github.com/piplabs/story/blob/e6d2d51550c3eff3f561f8d1b860888ea2bf8060/contracts/src/protocol/IPTokenStaking.sol#L446

    function redelegateOnBehalf(
        bytes calldata delegatorUncmpPubkey,
        bytes calldata validatorUncmpSrcPubkey,
        bytes calldata validatorUncmpDstPubkey,
        uint256 delegationId,
        uint256 amount
    )
        external
        verifyUncmpPubkey(delegatorUncmpPubkey)
        verifyUncmpPubkey(validatorUncmpSrcPubkey)
        verifyUncmpPubkey(validatorUncmpDstPubkey)

It's recommended to add the chargeFee modifier to them.

SOLVED: The Story team fixed this issue as recommended.

The function isNextProposer attempts to determine whether the local node is the next proposer for the upcoming block in a CometBFT consensus system. However, there is a fundamental issue with the logic: it incorrectly applies a simple round-robin algorithm to select the next proposer, while CometBFT uses a weighted round-robin algorithm based on validator voting power. This discrepancy can lead to incorrect proposer selection, which may cause the node to behave incorrectly in the consensus process, potentially leading to missed blocks or other consensus failures:

// isNextProposer returns true if the local node is the proposer
// for the next block. It also returns the next block height.
//
// Note that the validator set can change, so this is an optimistic check.
func (k *Keeper) isNextProposer(ctx context.Context, currentProposer []byte, currentHeight int64) (bool, error) {
	// PostFinalize can be called during block replay (performed in newCometNode),
	// but cmtAPI is set only after newCometNode completes (see app.SetCometAPI), so a nil check is necessary.
	if k.cmtAPI == nil {
		return false, nil
	}

	valset, ok, err := k.cmtAPI.Validators(ctx, currentHeight)
	if err != nil {
		return false, err
	} else if !ok || len(valset.Validators) == 0 {
		return false, errors.New("validators not available")
	}

	idx, _ := valset.GetByAddress(currentProposer)
	if idx < 0 {
		return false, errors.New("proposer not in validator set")
	}

	nextIdx := int(idx+1) % len(valset.Validators)
	nextProposer := valset.Validators[nextIdx]
	nextAddr, err := k1util.PubKeyToAddress(nextProposer.PubKey)
	if err != nil {
		return false, err
	}

	isNextProposer := nextAddr == k.validatorAddr

	return isNextProposer, nil
}

The code uses a modulo operation (nextIdx := int(idx+1) % len(valset.Validators)) to select the next proposer in a round-robin fashion. However, CometBFT uses a weighted round-robin algorithm that takes into account the voting power of each validator when selecting proposers. This oversight means that validators with higher voting power will not be appropriately prioritized, resulting in an incorrect proposer selection.

Modify the proposer selection logic to account for validator voting power in line with CometBFT’s weighted round-robin algorithm, as it is explained in CometBFT’s documentation.

SOLVED: The Story team fixed this issue by using the exposed validators functions to check if the local node is the next (predicted) proposer.

There is a possible non-determinism issue in FinalizeBlock. That function must be deterministic per the CometBFT specs. However, there is a call path postFinalize -> isNextProposer -> Validators -> cmtAPI that can incurr in a non-deterministic scenario:

https://github.com/piplabs/story/blob/e6d2d51550c3eff3f561f8d1b860888ea2bf8060/client/app/abci.go#L114

// FinalizeBlock calls BeginBlock -> DeliverTx (for all txs) -> EndBlock.
func (l abciWrapper) FinalizeBlock(ctx context.Context, req *abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) {

	...

	if err := l.postFinalize(sdkCtx); err != nil {
		log.Error(ctx, "PostFinalize callback failed [BUG]", err, "height", req.Height)
		return resp, err
	}

        ...

https://github.com/piplabs/story/blob/e6d2d51550c3eff3f561f8d1b860888ea2bf8060/client/x/evmengine/keeper/abci.go#L189

// PostFinalize is called by our custom ABCI wrapper after a block is finalized.
// It starts an optimistic build if enabled and if we are the next proposer.
//
// This custom ABCI callback is used since we need to trigger optimistic builds
// immediately after FinalizeBlock with the latest app hash
// which isn't available from cosmosSDK otherwise.
func (k *Keeper) PostFinalize(ctx sdk.Context) error {

	...

	// Maybe start building the next block if we are the next proposer.
	isNext, err := k.isNextProposer(ctx, proposer, height)
	if err != nil {
		return errors.Wrap(err, "next proposer")
	}

        ... 

https://github.com/piplabs/story/blob/e6d2d51550c3eff3f561f8d1b860888ea2bf8060/client/x/evmengine/keeper/keeper.go#L191

// isNextProposer returns true if the local node is the proposer
// for the next block. It also returns the next block height.
//
// Note that the validator set can change, so this is an optimistic check.
func (k *Keeper) isNextProposer(ctx context.Context, currentProposer []byte, currentHeight int64) (bool, error) {

	...

	valset, ok, err := k.cmtAPI.Validators(ctx, currentHeight)
	if err != nil {
		return false, err
	}

        ...

The reason why it can lead to non-determinism is because the call to the CometBFT API is done through an RPC, which is a network connection, which by definition is non-deterministic between nodes (as some can revert due to timeout, network errors or go on flawlessly). That makes it possible that, under the same input, some nodes would go on whilst others would crash, as the errors are not handled in the FinalizeBlock context but rather bubbled up to the caller, leading to a chain split and the consensus would be broken.

Ignore any error returned from PostFinalize in FinalizeBlock.

SOLVED: The Story team fixed this issue by skipping optimistic builds if any non-determinism error was encountered.

The function UncmpPubKeyToCmpPubKey is responsible for converting uncompressed public keys into their compressed format. While it correctly handles the conversion by extracting the x and y coordinates and determining the prefix based on the parity of y, it does not validate whether the provided public key lies on the elliptic curve. This omission can lead to unexpected errors or security vulnerabilities when other modules, such as evmstaking, attempt to use these keys.

https://github.com/piplabs/story/blob/e6d2d51550c3eff3f561f8d1b860888ea2bf8060/client/x/evmstaking/keeper/keys.go#L14-L35

func UncmpPubKeyToCmpPubKey(uncmpPubKey []byte) ([]byte, error) {
	if len(uncmpPubKey) != 65 || uncmpPubKey[0] != 0x04 {
		return nil, errors.New("invalid uncompressed public key length or format")
	}

	// Extract the x and y coordinates
	x := new(big.Int).SetBytes(uncmpPubKey[1:33])
	y := new(big.Int).SetBytes(uncmpPubKey[33:])

	// Determine the prefix based on the parity of y
	prefix := byte(0x02) // Even y
	if y.Bit(0) == 1 {
		prefix = 0x03 // Odd y
	}

	// Construct the compressed public key
	compressedPubKey := make([]byte, 33)
	compressedPubKey[0] = prefix
	copy(compressedPubKey[1:], x.Bytes())

	return compressedPubKey, nil
}

As it can be seen above, the function does not verify whether the x and y coordinates of the uncompressed public key satisfy the elliptic curve equation. Public keys that do not lie on the curve are invalid and can cause cryptographic operations to fail unexpectedly in downstream modules. Since this function is used by other modules, any invalid key processed here could lead to cascading failures or unexpected behavior in those modules.

It is recommended to validate whether the used points are in the described curve in order to discard incorrect keys.

SOLVED: The Story team solved these issues as it was recommended.

Smart contracts in scoped folder use the floating pragma >=0.8.23. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, either an outdated compiler version that might introduce bugs that affect the contract system negatively or a pragma version too new which has not been extensively tested.

Consider locking the pragma version with known bugs for the compiler version by removing the >= symbol. When possible, do not use floating pragma in the final live deployment. Specifying a fixed compiler version ensures that the bytecode produced does not vary between builds. This is especially important if you rely on bytecode-level verification of the code.

SOLVED: The Story team fixed this issue by setting 0.8.23 as fixed pragma version.

Failed operations in several contracts are reverted with an accompanying message selected from a set of hard-coded strings.

In the EVM, emitting a hard-coded string in an error message costs ~50 more gas than emitting a custom error. Additionally, hard-coded strings increase the gas required to deploy the contract.

It's recommended to use custom errors instead of hardcoded strings.

ACKNOWLEDGED: The Story team acknowledged this issue by stating the following:

Acknowledged, deployment costs are not an issue due to those contracts being predeploys. We will consider using custom errors for mainnet.

It was identified that no high-privileged user can pause any of the scoped contracts. In the event of a security incident, the owner would not be able to stop any plausible malicious actions. Pausing the contract can also lead to more considered decisions.

It is recommended including the pause feature in the contracts.

ACKNOWLEDGED: The Story team acknowledged this issue.