Bugs / vulnerabilities due to cross contract calls

[tamirms]

In ethereum although there is a standard ERC20 interface for tokens, not all token implementations conform to the standard. For example, some tokens revert when a transfer fails, while other tokens do not revert and instead return a false boolean indicating the transfer did not succeed. Differences among the ERC20 implementations have caused bugs in smart contracts which attempt to interact with any ERC20 token (e.g. uniswap).

The SafeERC20 library helps mitigate these issues but that is not fool proof. Consider the transfer(dest, amount) function, you would expect that, if the call succeeded, amount tokens will always be transferred to dest. However, a token contract might choose to charge potential fee on every transfer which is deducted from amount. A contract relying on transfer(dest, amount) always transferring amount tokens would potentially be broken in this scenario.

Generally speaking, if there are any assumptions about how another contract behaves and those assumptions are broken it can result in security vulnerabilities. Reentrancy attacks are a good example of this.

The reason I bring this up is that Soroban has a builtin Stellar Asset Contract and it is very easy to assume that, when you have a token client, you are interacting with the builtin Stellar Asset Contract. But that would be a false assumption because, the token client works on any Soroban contract which conforms to the token trait. So, even though there is a single Stellar Asset Contract implementation, the Soroban smart contract developer still needs to be aware of all the pitfalls from the ethereum world that I described above.

I believe this issue can also be compounded by Auth next. Consider the Soroban <-> Ethereum bridge which requires validators to sign a transaction that would allow participants to claim a cross-chain payment. In the Auth next world, validators need to be very careful when signing transactions that would call the xfer() function on an arbitrary token contract. An attacker can craft a malicious contract which conforms to the token interface and uses the validator's authorization to steal funds from the bridge.

I think we could mitigate these threats by providing the option to create a contract client which asserts the contract conforms to a specific implementation (e.g. create a token client that asserts the token contract is the single asset contract).

More importantly we should include several Soroban examples which exhibit defensive smart contract practices. For example, I don't think we have any examples in our documentation of how to make contracts robust against reentrancy attacks.

[dmkozh]

I think we could mitigate these threats by providing the option to create a contract client which asserts the contract conforms to a specific implementation

I'm not sure about the exact interface, but we'll provide a way to determine what is the contract Wasm hash and whether it's a built-in contract.

An attacker can craft a malicious contract which conforms to the token interface and uses the validator's authorization to steal funds from the bridge.

I'm not sure I understand this. How do validators sign the transactions? (IIUC that's not something we have support for) More importantly, how exactly would a malicious token contract steal any funds? Wouldn't it need to be also bridged?

For example, I don't think we have any examples in our documentation of how to make contracts robust against reentrancy attacks.

That's because reentrancy is simply not allowed currently. When/if we enable it, we'll try to make sure things are documented.

In general, while I understand these concerns, I wonder how much we can/should do to prevent them. Basically most of the issues are about not following the token interface. It's not obvious to me that 'bugged' tokens would get much value/use out of them (I've no idea why is this the case in the ETH world). I can always write a token that would redirect all the transfers to my address, but why would anyone ever use it?

[C0x41lch0x41]

An attacker can craft a malicious contract which conforms to the token interface and uses the validator's authorization to steal funds from the bridge.

I'm not sure I understand this. How do validators sign the transactions? (IIUC that's not something we have support for) More importantly, how exactly would a malicious token contract steal any funds? Wouldn't it need to be also bridged?

+1 to @dmkozh point. If I understand correctly the bridge design, bridge validators would need to add support for each coin that is bridged. So it is unlikely that a buggy token will be accepted.
I also think Auth next has the unique capability of letting the user accept exactly which contract is used. That prevents a user to blindly authorize a proxy that calls a malicious contract.

[tamirms]

@dmkozh

I'm not sure I understand this. How do validators sign the transactions? (IIUC that's not something we have support for) More importantly, how exactly would a malicious token contract steal any funds? Wouldn't it need to be also bridged?

Here is some context on how the bridge works: #635 (comment)

By "validator" I meant the bridge validator which you can think of as the "admin" for the bridge smart contracts. In order to withdraw assets from the bridge soroban smart contract the "bridge admin" needs to authorize the call to the withdraw function:

https://github.com/stellar/starbridge/blob/soroban/soroban-bridge/src/lib.rs#L78

Currently, this authorization is implemented setting the source account for the invoke host operation to the multisig "bridge admin" stellar account:

https://github.com/stellar/starbridge/blob/soroban/stellar/signer.go#L33-L76

The bridge admin basically creates the transaction and calls the preflight endpoint to populate the footprint. Although the source account for the operation is the bridge admin, the source account for the entire transaction is the relayer who is paying for the transaction fees and using its account sequence number to deliver the asset to the recipient.

Because the authorization check in the sorban contract is simply comparing the invoker against the bridge admin recorded in the storage, the withdraw workflow should be safe.

But, if we're using auth next to determine that the withdraw call is authorized by the bridge admin we need to be more careful because we can't blindly use the authorizations from the preflight endpoint. The point I'm trying to make is that the code to approve bridge withdrawals needs to be careful of whatever authorizations are hidden behind the token contract.

[tamirms]

@C0x41lch0x41

If I understand correctly the bridge design, bridge validators would need to add support for each coin that is bridged. So it is unlikely that a buggy token will be accepted.

That's not entirely correct. The bridge contract allows anyone to deposit any type of asset to the bridge. However, only a restricted set of assets can be actually delivered to the Ethereum end of the bridge. So, what happens if you deposit an unsupported asset? You won't be able to withdraw it on Ethereum but you can still collect your refund after the time lock expires.

We could change the bridge design so that deposits only succeed if the asset is in the restricted set but this design will use more gas and more storage compared to the current design of keeping the restricted allow list completely off-chain.

But, for the sake of argument, let's say we make deposits restricted. In a world where contracts are allowed to be upgraded it is possible for the bridge to include what seems to be a "safe" asset contract in the restricted set but, at a later point the asset contract could become compromised and upgraded to a version which is malicious.

[dmkozh]

By "validator" I meant the bridge validator which you can think of as the "admin"

Thanks for the explanation! I was under the impression that Stellar validator needs to sign something.

But, if we're using auth next to determine that the withdraw call is authorized by the bridge admin we need to be more careful because we can't blindly use the authorizations from the preflight endpoint.

I'm not sure I understand the concern here. Auth next doesn't prohibit you from authorization via checking the admin address. It also allows you to decouple the transaction source account from the auth, so that e.g. bridge can skip paying fees. With the updated auth next interface that I'm working now you'll be able to do things like env.require_auth(env.storage().get(DataKey::Admin), /*args*/) and your bridge would need to only return the signature for this specific auth check.

You won't be able to withdraw it on Ethereum but you can still collect your refund after the time lock expires.

This sounds like a reasonable approach, but what is the concern/vulnerability if it's implemented?

[C0x41lch0x41]

In a world where contracts are allowed to be upgraded it
@dmkozh this is a more generic question, but what happens with Auth next if/when we allow contract to be updated? If you authorize contract A to call function fn(a, b, c) and then contract A is updated but fn keeps the same interface fn(a, b, c), will the authorization still be valid?

[dmkozh]

If you authorize contract A to call function fn(a, b, c) and then contract A is updated but fn keeps the same interface fn(a, b, c), will the authorization still be valid?

Yes, for now we wouldn't invalidate the signature. It's not impossible to add the implementation pointer to the signature payload, but I'm not convinced if that's generically useful. Maybe as an optional argument for some specific use cases? Enforcing this is also tricky as compromised contract can disable its own auth.

IMHO the risk surface doesn't change significantly if we keep signatures valid. If the contract implementation is compromised, there is really no need to do anything fancy with user authorizations as it's just possible to transfer the contract's funds anywhere. Also note, that while the signature will stay valid, it's not possible to add/modify the subcontract calls. So if you've signed withdrawal of 10 XLM no matter how implementation changes host won't allow to withdraw more than 10 XLM from you.

[tamirms]

I'm not sure I understand the concern here. Auth next doesn't prohibit you from authorization via checking the admin address.

Assume we have the following simplified withdraw function on the bridge which requires authorization from the admin address:

pub fn withdraw(env: Env, token: BytesN<32>, recipient: Identifier, id: BytesN<32>, amount: i128) {
        check_admin(&env, &env.invoker().into());

        let key = DataKey::Fullfilled(id);
        if env.storage().has(&key) {
            panic!("withdrawal already fulfilled");
        } else {
            env.storage().set(&key, ());
        }

        let client = token::Client::new(&env, &token);
        client.xfer(&env.current_contract(), &recipient, &amount)
}

pub fn check_admin(e: &Env, admin: &Identifier) {
    if *admin != get_admin(e) {
        panic!("not authorized by admin")
    }
}

The scenario I had in mind is that the token contract is defined with a malicious xfer() function which will call xfer() on the xlm stellar asset contract to steal the lumens held by the bridge.

The validator should not blindly use the invocations from the preflight endpoint. It needs to make sure the invocations don't include anything suspicious.

[dmkozh]

that the token contract is defined with a malicious xfer() function which will call xfer() on the xlm stellar asset contract to steal the lumens held by the bridge.

This is not possible. Both currently and with auth next contract only authorizes the direct calls on its behalf (we may probably extend that, but that won't ever happen implicitly). Hence the contract has to explicitly call xfer on the XLM token.

The validator should not blindly use the invocations from the preflight endpoint. It needs to make sure the invocations don't include anything suspicious.

This is true for any signature and is a bit orthogonal to auth next (e.g. with your implementation the bridge could just accidentally sign the XLM withdrawal... but it shouldn't do that unless there are bugs). FWIW there is really no need to run preflight for every call and especially for the cases when on one hand the signature payload is stable and well-defined and on the other hand there are certain security risks due to automated signing (which is different from manual signing with the wallet). So the right thing to do here with auth next would probably be to build the signature payload manually and then sign it either separately or as a part of the whole transaction (at which point it would be logical for the bridge to just run the transaction).

[tamirms]

This is not possible. Both currently and with auth next contract only authorizes the direct calls on its behalf (we may probably extend that, but that won't ever happen implicitly). Hence the contract has to explicitly call xfer on the XLM token.

The auth next design doc mentioned a tree of contract invocations and in your example there is a sample invocation listed with multiple invocations across different contract ids:

footprint: ...
accountAuthorizations: [
	{
		// The address is inferred from the source account of the preflight transaction		
		address: ScAddress::ClassicAccount('GA...')
		invocations: [
			{
				callStack: [
					{ contractId: '$contract_id', fn: 'deposit'}
				],
				args: [
					/*amount*/100, /*another deposit args*/
				],
				nonce: Some(123)
			},
			{
				callStack: [
					{ contractId: '$contract_id', fn: 'deposit'},
					{ contractId: '$token_id', fn: 'xfer'},
				],
				args: [
					/*to*/ '$contract_id',
					/*amount*/100
				],
				nonce: None
			},
		]
	}
]

Note the callstack in the 2nd invocation has two different contract ids. Maybe I'm misunderstanding what you mean by "only authorizes the direct calls on its behalf" ?

This is true for any signature and is a bit orthogonal to auth next

My point is that it's easier to authorize a top level invocation but when you have to analyze the behavior of other contracts which are invoked from the top level contract then it becomes more difficult.

[dmkozh]

there is a sample invocation listed with multiple invocations across different contract ids:

Exactly, in this case we authorize an xfer of 100 units of $token_id. If $token_id is not XLM, then it's not possible to withdraw XLM from me, no matter what $token_id implementation does.

My point is that it's easier to authorize a top level invocation but when you have to analyze the behavior of other contracts which are invoked from the top level contract then it becomes more difficult.

You don't have to analyze behavior of the other contracts. If your intent is to always authorize the top-level call only, you can always do that by either building your invocation data manually (which is trivial for the case of a single top-level invocation and is easy to build helpers for) or by verifying that there is only a single invocation in the preflight response.