Canto contest - hihen's results

Lines of code

https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/Canto/x/csr/keeper/evm_hooks.go#L51-L86

Vulnerability details

Impact

The incorrect way revenue is calculated can lead to CSR being stolen through proxy attacks, which is likely to lead the ecology into CSR bribery war.

Eventually, this feature will translate into reduced gas fees for all transactions, regardless of whether contracts are registered for the CSR or not.

This amounts to a complicated way to change the transaction gas fee calculation formula. A discount for everyone is a discount for no one.

Proof of Concept

In PostTxProcessing, the CSR fee is sent to the nftID associated with the contract msg.To().

This means, in the case of a transaction with external calls EOA -> C1 -> C2 -> C3 ..., contract C1 will get the entire CSR fee of the transaction.

For any existing contract or application, an attacker can deploy a proxy contract to capture all the CSR fees that would otherwise belong to that contract / application developer. The proxy contract will register with csr and use part of the revenue to bribe users to use it.

Therefore, for any deployed or undeployed dapp, the only way to prevent this attack is to modify the contract or deploy the proxy itself to return most of the revenue to users.

That's not enough. Eventually users will find that they don't need to care if the dapp they interact with registered for the CSR and returns all revenue. Because they can get all CSR fee by making all their transactions through their own proxy contracts.

Yes, the proxy is just like a smart wallet, and all users will love using a smart wallet.

However, this does not meet the design purpose of the canto CSR:

We present a novel economic mechanism which modifies EIP-1559 to distribute a portion of the total base fee (an amount that would otherwise be burnt) to the deployers of the contracts that consume gas within a given block.

The Canto CSR Store is a revenue-sharing-per-transaction model that allows smart contract developers to accumulate revenue to a tradable NFT.

Tools Used

VS Code

Recommended Mitigation Steps

Real contract developers should be protected from CSR fee theft.

When sending CSR fee, not the entire CSR fee should be sent directly to the top contract(the to of the tx), but to every contract that is invoked.

This can be calculated and sent separately by taking the evm's call stack and gas consumption per contract call(internal transaction).

Lines of code

https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/Canto/x/csr/keeper/evm_hooks.go#L49 https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/Canto/x/csr/keeper/evm_hooks.go#L101-L135

Vulnerability details

Impact

Some contracts and some Turnstile tokens (nfts) wll not be able to receive CSR fees forever.

Proof of Concept

In evm_hooks.go, the PostTxProcessing will call h.processEvents(ctx, receipt) to handle Register and Assign events from Turnstile contract first:

h.processEvents(ctx, receipt)

Notice that the processEvents function does not return any error.

However, it is possible for processEvents to encounter an error:

func (h Hooks) processEvents(ctx sdk.Context, receipt *ethtypes.Receipt) {
  ...
  for _, log := range receipt.Logs {
    ...
    if log.Address == turnstileAddress {
      ...
      switch event.Name {
      case types.TurnstileEventRegister:
        err = h.k.RegisterEvent(ctx, log.Data)
      case types.TurnstileEventUpdate:
        err = h.k.UpdateEvent(ctx, log.Data)
      }
      if err != nil {
        h.k.Logger(ctx).Error(err.Error())
        return
      }
    }
  }
}

According to the above implementation of processEvents, it will process all the events emitted by the transaction one by one. If one of them encounters an error, it will return directly without any error, and any subsequent unprocessed events will be ignored.

Suppose we have a transaction contains the following events(by contract calls):

1. Register C1 with token1
2. Register C2 with token2
3. Assign C3 with token1

If RegisterEvent() returns an error when handling the first event, then all of the events will not be handled because processEvents() will return after logging the error. And PostTxProcessing() continues to execute normally because it is unaware of the error.

According to the current implementation of RegisterEvent() and UpdateEvent, they are both easy to encounter an error. Like register() using a recipient that doesn't exist yet.

As a result, none of the C1, C2, C3 contracts will be able to recieve any CSR fee because they are not recorded in csr store.

Contrcts C1, C2, C3 will never be able to register for CSR because they are marked registered in Turnstile contract (evm store) and will be reverted by onlyUnregistered when calling register() or assign().

And all other contracts calling assign(token1) or assign(token2) will enter the same state as C1/C2/C3, because the assign() will succeed in Turnstile contract but fail in UpdateEvent() (because the store can not find token1 or token2):

  // Check if the NFT that is being updated exists in the CSR store
  nftID := event.TokenId.Uint64()
  csr, found := k.GetCSR(ctx, nftID)
  if !found {
  	return sdkerrors.Wrapf(ErrNFTNotFound, "EventHandler::UpdateEvent the nft entered does not currently exist: %d", nftID)
  }

Tools Used

VS Code

Recommended Mitigation Steps

processEvents() should return the error it encounters, and PostTxProcessing() should return that error too.

Lines of code

https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/CIP-001/src/Turnstile.sol#L149

Vulnerability details

Impact

Transactions may fail because of this redundant check.

Proof of Concept

The function distributeFees will revert if msg.value == 0:

function distributeFees(uint256 _tokenId) public onlyOwner payable {
    if (msg.value == 0) revert NothingToDistribute();

    balances[_tokenId] += msg.value;
    emit DistributeFees(_tokenId, msg.value);
}

According to evm_hooks.go - PostTxProcessing():

  fee := sdk.NewIntFromUint64(receipt.GasUsed).Mul(sdk.NewIntFromBigInt(msg.GasPrice()))
  csrFee := sdk.NewDecFromInt(fee).Mul(params.CsrShares).TruncateInt()
  evmDenom := h.k.evmKeeper.GetParams(ctx).EvmDenom
  csrFees := sdk.Coins{{Denom: evmDenom, Amount: csrFee}}

it's possible for msg.value == 0 when calling distributeFees() from evm_hooks.go:

1. if CsrShares is set to 0 for some reason
2. if the transaction (msg) has a gas price of 0

When that happened, the distributeFees() will revert, causing PostTxProcessing() to return with an error, causing the whole evm transaction being reverted.

Tools Used

VS Code

Recommended Mitigation Steps

Should delete Turnstile.sol#L149.