Users might fall victims of a false positive: if they use a fresh account as an NFT recipient during contract registration, the transaction won’t revert, but the registered contract will never earn fees for the token holder. And since a contract can be registered only once, there won’t be a way for affected users to re-register contracts and start earning fees. This can affect both big and smaller project that register their contracts with the Turnstile contract: the only condition for the bug to happen is that the recipient address that’s used during registration is a fresh address (i.e. an address that hasn’t been used yet).
The register function allows the calling contract to specify the address that will receive the freshly minted NFT (Turnstile.sol#L86):
function register(address _recipient) public onlyUnregistered returns (uint256 tokenId) {
address smartContract = msg.sender;
if (_recipient == address(0)) revert InvalidRecipient();
tokenId = _tokenIdTracker.current();
_mint(_recipient, tokenId);
_tokenIdTracker.increment();
emit Register(smartContract, _recipient, tokenId);
feeRecipient[smartContract] = NftData({
tokenId: tokenId,
registered: true
});
}
A recipient address can be any address besides the zero address. However, on the consensus layer, there’s a stricter requirement (event_handler.go#L31-L33): a recipient address cannot be a fresh account, that is an address that:
While, on the application layer, calling the register function with a fresh address will succeed, on the consensus layer a contract won’t be registered. When a Register event is processed on the consensus layer, there’s a check that requires that the recipient address is an existing account in the state database (event_handler.go#L31-L33):
// Check that the receiver account exists in the evm store
if acct := k.evmKeeper.GetAccount(ctx, event.Recipient); acct == nil {
return sdkerrors.Wrapf(ErrNonexistentAcct, "EventHandler::RegisterEvent account does not exist: %s", event.Recipient)
}
If the recipient account doesn’t exist, the function will return, but the register transaction won’t revert (errors during the events processing doesn’t result in a revert: evm_hooks.go#L123-L132, evm_hooks.go#L49).
The GetAccount function above returns nil when an address doesn’t exist in the state database. To see this, we need to unwind the GetAccount execution:
the GetAccount is called on an evmKeeper (event_handler.go#L31):
if acct := k.evmKeeper.GetAccount(ctx, event.Recipient); acct == nil {
return sdkerrors.Wrapf(ErrNonexistentAcct, “EventHandler::RegisterEvent account does not exist: %s”, event.Recipient)
}
evmKeeper is set during the CSR Keeper initialization (keeper.go#L27):
func NewKeeper(
cdc codec.BinaryCodec,
storeKey sdk.StoreKey,
ps paramtypes.Subspace,
accountKeeper types.AccountKeeper,
evmKeeper types.EVMKeeper,
bankKeeper types.BankKeeper,
FeeCollectorName string,
) Keeper {
// set KeyTable if it has not already been set
if !ps.HasKeyTable() {
ps = ps.WithKeyTable(types.ParamKeyTable())
}
return Keeper{
storeKey: storeKey,
cdc: cdc,
paramstore: ps,
accountKeeper: accountKeeper,
evmKeeper: evmKeeper,
bankKeeper: bankKeeper,
FeeCollectorName: FeeCollectorName,
}
}
the CSR Keeper is initialized during the main app initialization (app.go#L473-L478), this is also when the EVM Keeper is initialized (app.go#L409-L413):
app.EvmKeeper = evmkeeper.NewKeeper(
appCodec, keys[evmtypes.StoreKey], tkeys[evmtypes.TransientKey], app.GetSubspace(evmtypes.ModuleName),
app.AccountKeeper, app.BankKeeper, &stakingKeeper, app.FeeMarketKeeper,
tracer,
)
the EVM Keeper is implemented and imported from Ethermint (keeper.go#L67);
here’s the GetAccount function (statedb.go#L25):
func (k *Keeper) GetAccount(ctx sdk.Context, addr common.Address) *statedb.Account {
acct := k.GetAccountWithoutBalance(ctx, addr)
if acct == nil {
return nil
}
acct.Balance = k.GetBalance(ctx, addr)
return acct
}
the GetAccountWithoutBalance function calls GetAccount on accountKeeper (keeper.go#L255-L258):
acct := k.accountKeeper.GetAccount(ctx, cosmosAddr)
if acct == nil {
return nil
}
The Account Keeper is implemented in the Cosmos SDK (account.go#L41-L49):
func (ak AccountKeeper) GetAccount(ctx sdk.Context, addr sdk.AccAddress) types.AccountI {
store := ctx.KVStore(ak.storeKey)
bz := store.Get(types.AddressStoreKey(addr))
if bz == nil {
return nil
}
return ak.decodeAccount(bz)
}
it basically reads an account from the store passed in the context object (context.go#L280-L282);
in the Account Keeper, there’s also SetAccount function (account.go#L72), and it’s called in Ethermint by the EVM Keeper (statedb.go#L126);
the EVM Keeper’s SetAccount is called when transaction changes are committed to the state database (statedb.go#L449);
the state database is a set of state objects, where keys are account addresses and values are accounts themselves;
the getOrNewStateObject function initializes new state objects (statedb.go#L221-L227);
getOrNewStateObject is only called by these functions: AddBalance, SubBalance, SetNonce, SetCode, SetState (statedb.go#L290-L328).
Thus, a new account object in the state database is only created when an address receives native coins, sends a transaction (which increases the nonce), or when contract code is deployed at it.
Manual review
Consider removing the “only existing recipient account” check in the RegisterEvent handler since it creates a discrepancy between the application and the consensus layers. Otherwise, if it’s mandatory that receiver addresses are not fresh, consider returning an error in the PostTxProcessing hook (which will revert a transaction) if there was an error during events processing.
The text was updated successfully, but these errors were encountered:
👍 1 0xSorryNotSorry reacted with thumbs up emoji ❤️ 1 0xSorryNotSorry reacted with heart emoji
All reactions