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

[Bug] The wasmd querier GasConsumed() is not adaptive wasmvm, maybe cause an attack。 #1536

Closed
lyh169 opened this issue Jul 28, 2023 · 2 comments · Fixed by #1567
Closed

Comments

@lyh169
Copy link

lyh169 commented Jul 28, 2023

  1. in wasmd
    when Execute, need generate the querier.
	// prepare querier
	querier := k.newQueryHandler(ctx, contractAddress)
	gas := k.runtimeGasForContract(ctx)
	res, gasUsed, execErr := k.wasmVM.Execute(codeInfo.CodeHash, env, info, msg, prefixStore, cosmwasmAPI, querier, k.gasMeter(ctx), gas, costJSONDeserialization)
	k.consumeRuntimeGas(ctx, gasUsed)
	if execErr != nil {
		return nil, errorsmod.Wrap(types.ErrExecuteFailed, execErr.Error())
	}

the newQueryHandler generate a object QueryHandler that implement the Querier interface as follow

type Querier interface {
	Query(request QueryRequest, gasLimit uint64) ([]byte, error)
	GasConsumed() uint64
}

the QueryHandler Gasconsumed is as follow.

func (q QueryHandler) GasConsumed() uint64 {
	return q.Ctx.GasMeter().GasConsumed()
}
  1. But in wasmvm the cQueryExternal get the GasConsumed as follow,
func cQueryExternal(ptr *C.querier_t, gasLimit cu64, usedGas *cu64, request C.U8SliceView, result *C.UnmanagedVector, errOut *C.UnmanagedVector) (ret C.GoError) {
	defer recoverPanic(&ret)
        
........
	// query the data
	querier := *(*Querier)(unsafe.Pointer(ptr))
	req := copyU8Slice(request)

	gasBefore := querier.GasConsumed()
	res := types.RustQuery(querier, req, uint64(gasLimit))
	gasAfter := querier.GasConsumed()
	*usedGas = (cu64)(gasAfter - gasBefore)
........
	return C.GoError_None
}

the *usedGas will be acquired by wasmvm/libwasmvm/src/querier.rs/query_raw, as follow

       &self,
       request: &[u8],
       gas_limit: u64,
   ) -> BackendResult<SystemResult<ContractResult<Binary>>> {
       let mut output = UnmanagedVector::default();
       let mut error_msg = UnmanagedVector::default();
       let mut used_gas = 0_u64;
       let go_result: GoError = (self.vtable.query_external)(
           self.state,
           gas_limit,
           &mut used_gas as *mut u64,
           U8SliceView::new(Some(request)),
           &mut output as *mut UnmanagedVector,
           &mut error_msg as *mut UnmanagedVector,
       )
       .into();
       // We destruct the UnmanagedVector here, no matter if we need the data.
       let output = output.consume();

       let gas_info = GasInfo::with_externally_used(used_gas);
.........
}

the gas_info will return to the cosmwasm, the process in process_gas_info

pub fn process_gas_info<A: BackendApi, S: Storage, Q: Querier>(
    env: &Environment<A, S, Q>,
    info: GasInfo,
) -> VmResult<()> 

because of this gasUsed is calculate in the sdk gas mode but not WasmVMGas, so it maybe influence the exec。

  1. this gasUsed calculate is not the same as the store 。

the store used GasMeter it GasConsumed() function as follow。

type MultipliedGasMeter struct {
	originalMeter sdk.GasMeter
	GasRegister   GasRegister
}

func NewMultipliedGasMeter(originalMeter sdk.GasMeter, gr GasRegister) MultipliedGasMeter {
	return MultipliedGasMeter{originalMeter: originalMeter, GasRegister: gr}
}

var _ wasmvm.GasMeter = MultipliedGasMeter{}

func (m MultipliedGasMeter) GasConsumed() sdk.Gas {
	return m.GasRegister.ToWasmVMGas(m.originalMeter.GasConsumed())
}

Is this GasConsumed() correct or not ? @alpe Thanks

@lyh169 lyh169 changed the title [Bug] The querier GasConsumed() maybe not correct, maybe cause an attack。 [Bug] The wasmd querier GasConsumed() is not adaptive wasmvm, maybe cause an attack。 Aug 2, 2023
@pinosu
Copy link
Contributor

pinosu commented Aug 3, 2023

Thanks for bringing this up! 🙏 I am not confident on giving an answer about this. @alpe is on holidays at the moment but he can probably help on this when he is back!

@webmaster128
Copy link
Member

Thanks a lot – looks like a great find. We are working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants