IOP _ ThunderNFT 34629 - [Smart Contract - Critical] Theft of Deposited Funds
Theft of Deposited Funds
Submitted on Sun Aug 18 2024 18:33:15 GMT-0400 (Atlantic Standard Time) by @Blockian for IOP | ThunderNFT
Report ID: #34629
Report type: Smart Contract
Report severity: Critical
Target: https://github.com/ThunderFuel/smart-contracts/tree/main/contracts-v1/thunder_exchange
Impacts:
Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield
Direct theft of any user NFTs, whether at-rest or in-motion, other than unclaimed royalties
Description
Thunder Exchange
Theft of Deposited Funds
Description
A critical vulnerability exists that enables a malicious actor to steal all deposited non-unique tokens (e.g., tokens following the ERC1155 standard) listed for sale.
Root Cause
As discussed in our Discord exchange, the Order.amount field was introduced to accommodate ERC1155-style tokens:
Hi! Yes, amount is added in case of Erc1155 style token standard
However, when updating a sell order, there is no validation to ensure that the order maker has deposited the required additional tokens into the exchange.
In the code snippet below, you can see that the update_order function does not check if additional tokens have been deposited when modifying an existing order:
This omission allows an attacker to modify the amount variable in the order without actually depositing the corresponding tokens.
For comparison, the place_order function does perform this validation:
Impact
This vulnerability permits an attacker to artificially increase the amount in an order, and subsequently invoke the cancel_order function.
As a result, the exchange sends the inflated number of tokens to the attacker, effectively enabling theft from legitimate users.
Proposed fix
To mitigate this issue, it is recommended to enforce the following during an order update:
Increase in Amount: Require the user to deposit additional tokens if the
amountis increased.Decrease in Amount: Refund the corresponding tokens to the user if the
amountis decreased.
Proof of concept
Proof of Concept
Due to the relatively new nature of the Sway language and the limited availability of reliable testing tools, the proof of concept (PoC) is complex.
Below is a pseudo-PoC followed by a detailed actual PoC.
Pseudo POC
An innocent user creates a Sell Order for some Asset
The malicious actor creates a Sell Order for the same Asset
The malicious actor updates the Sell Order amount to include the innocent user's deposit (for example, innocent user deposited
Xtokens, the malicious actor depositedYtokens, the malicious actor updates the Sell Order to beX + Y)The malicious actor cancels their Sell Order
Actual POC
There are some steps to follow:
To create an actual PoC, some modifications to the protocol are necessary.
The protocol currently only allows addresses to interact, which is generally fine. However, when transferring coins in the Fuel ecosystem, a Variable Output needs to be added to the transaction — this isn't supported by the current Sway testing tools.
Since this vulnerability is unrelated to the nature of who interacts with the protocol, adjustments will be made to allow contracts to interact, enabling the PoC. These changes are strictly for testing purposes and do not affect the core issue.
Changes to the protocol for the POC
Add the following changes to the
thunder_exchangecontract:
In addition, To make an MakerOrderInput Struct, we need to make the ExtraParams Struct public, so in the order_types.sw file, lets add a pub in line 37.
The tests files.
Create
forc.tomlincontracts-v1and add the below in theforc.toml:
Create 4 new folder called
tests,test_user,test_attacker, andtest_assetunder thecontracts-v1directory:In the each folder create a folder named
srcwith a file calledmain.sw, and aforc.tomlfile. The folder tree will look like this:
tests folder
In the tests folder.
Add the below in the
forc.toml:
Add the below in the
main.sw:
test_user folder
In the test_user folder.
Add the below in the
forc.toml:
Add the below in the
main.sw:
test_attacker folder
In the test_attacker folder.
Add the below in the
forc.toml:
Add the below in the
main.sw:
test_asset folder
In the test_asset folder. The test asset is simply the Fuel Team SRC3 example
Add the below in the
forc.toml:
Add the below in the
main.sw:
Interfaces
Now we need to add the test_user and test_attacker interfaces to interact with. In the interfaces/src folder, in the lib.sw add the following lines:
Additionally, create a file called test_user_interface.sw in the interfaces/src folder and add the following:
Create a file called test_attacker_interface.sw in the interfaces/src folder and add the following:
Run it all!
Simply run forc test in smart-contracts/contracts-v1.
POC TL;DR
Initializing the project contracts
Minting some coins to the
test_userandtest_attackerTest User creates an innocent Sell Order
Test Attacker steals Test User's deposited tokens
Last updated
Was this helpful?