stellar / soroban-examples

Example Soroban Contracts
Apache License 2.0
57 stars 60 forks source link

mint-lock contract allows minters to bypass their epoch limit. #295

Closed FredericRezeau closed 7 months ago

FredericRezeau commented 7 months ago

What version are you using?

Related to: https://github.com/stellar/soroban-examples/pull/294 (mint-lock)

What did you do?

The mint-lock contract example code contains a vulnerability that allows minters to bypass their epoch limit.

amount is a signed i128 value so a minter may use a mint contract, accepting negative values, to decrease their consumed_limit.

consumed_limit: minter_stats.consumed_limit + amount

Provide a recipe for reproducing the error.

1) Deploy a contract with mint function conforming to the MintInterface signature. The function can be a no-op, essentially enabling a successful invocation of the mint function with a negative amount value on the mint-lock contract.

2) Invoke the mint function on the mint-lock contract with any negative amount. This decreases the consumed_limit for the minter.

3) The epoch limit for the minter is now increased by the absolute value of the negative amount used.

Reproduced on Testnet:

set minter limit (100) 0b6adff4136337518af75943641d98c4fc6f854328e41f4ec4f7b6d7b97ee6e7 mint with negative amount (-10000000) 906d356f6e4559b7281a8e86e0bfb5cfe01ffd17b2fa8e89440e99a5c2fd60b2 mint with positive amount (10000000) 5b92f29860678da49fa230f6a46b5e6feb9491de4dc557f151ee8e732a79d61d

What did you expect?

Negative amounts should not be allowed. Could either assert amount > 0 or key the contract with the config so minter cannot use any minting contract.

leighmcculloch commented 7 months ago

Nice catch. I like the suggestion to disallow negative values. It's a broad way to limit impact from unintentional misuse. An exploit like this relies on a token that accepts negative mint values, which I think would be unlikely. But in any case this mint-lock contract could just deny it as well to ensure no internal misbehavior due to limits behavior with negative behaviors being not well defined.

leighmcculloch commented 7 months ago

Fix in:

FredericRezeau commented 7 months ago

The fix will do but I believe there's still a misunderstanding because the issue isn't just a case of unlikely unintentional misuse since the contract provides decoupling between the minting token and the minter limit. As demoed (testnet txs), a minter with malicious intent could purposefully exploit the vulnerability by minting negatively with a no-op token to reduce their global consumed_limit, allowing them to subsequently mint any regular token with a limit increased by the absolute value of their previous invocation.

For clarity of purpose (and since Soroban examples are likely to be used as reference by devs), I would also suggest that the system ties the minting limit to the token because amounts are likely contextual and only relevant to a specific token.

leighmcculloch commented 7 months ago

Ah yes, there's two issue and this second issue still exists. The admin of the mint lock contract doesn't control what the limit applies to.

leighmcculloch commented 7 months ago

Fix in:

FredericRezeau commented 7 months ago

Nice. On a separate topic, I see that you used multiple-value tuple variants for the enum in the contract so they seem allowed for contract types #[contracttype], but the Soroban documentation is saying that only unit and single-value variants are supported. Docs may need an update on this line.

leighmcculloch commented 7 months ago

Thanks! Fix in: