 |
Prepared by: Curiousapple, Independent Security Researcher Duration: 3.5 days, Mar 13, 2024 |
In their own words, "Nostra is the crypto Super App, powered by Starknet where users can lend, borrow, swap and bridge cryptocurrencies."
This review is for their new addition of Staked STRK.
Staked STRK is a ERC4626 vault with STRK as asset token and some added functionalities like withdrawal fee, deposit cap, pausability, and upgradability.
Curiousapple is an independent smart contract security researcher.
Previously, he served as a lead smart contract auditor at Macro and is currently working independently.
His auditing experience covers diverse set of protocols, including DeFi, Bridges, NFTs, DAOs, and Games, in all of which he has discovered severe bugs.
You can find his previous work and contact here
Repo: nostra-staked-strk
Contracts:
- src/shared/erc4626/erc4626.cairo
- src/shared/math.cairo
- src/staked_strk.cairo
Additional Disclaimer 🔴
This review solely focused on the logical aspects, leveraging Curiousapple's experience with DeFi, and did not consider the language-specific aspects of Cairo or Starknet.
As Curiousapple's experience with Cairo is limited, he wasn't comfortable reviewing contracts holistically on his own. Hence, he was contracted to focus only on logic, given his experience with DeFi.
Nostra is committed to comprehensive coverage, and as such, a subsequent review specifically dedicated to Cairo and Starknet language specifics will be conducted by a different reviewer.
| Acronym | |
|---|---|
| C | Critical |
| H | High |
| M | Medium |
| L | Low |
| Q | Quality |
| G | Gas |
| ID | Title | Status |
|---|---|---|
| H-01 | Withdrawals won’t work due to the incorrect order of state updates. | Fixed |
| Q-01 | max_mint() logic is unnecessarily complicated |
Fixed |
- Impact: Medium (Users could still execute withdrawls by
redeem()) - Likelihood: High
For withdrawals, Nostra updates total assets first and calculates the shares later.
This is detrimental, as by doing so, Nostra reduces the denominator for the exchange rate (total_supply/total_assets), inflating the shares to be burned.
Since the user won't have this many shares, it would revert all full withdrawals.
staked_stark.cairo
fn withdraw(
ref self: ContractState, assets: u256, receiver: ContractAddress, owner: ContractAddress
) -> u256 {
----
total assets update => self.total_assets.write(self.total_assets() - assets - fee_amount);
shares calculation => shares = self.erc4626.withdraw(assets, receiver, owner);
----
}Consider currently there are 1000 shares and 1000 strks with a withdrawal fee of 0% for simple calculations.
totalAssets=1000
totalSupply=1000
I own 100 assets and 100 shares.
I come in and call withdraw for my 100 assets.
withdraw(assets = 100)
step 1: fee_amount = 0 (L306)
step 2: total assets = 900 (L307)
step 3: erc4626.withdraw (100)
preview_withdraw(100)
erc4626.preview_withdraw(100)
erc4626._convert_to_shares(100)
(100 * 1001)/901 = 112.209766926
But I don't own this many shares
The problem is Nostra is reducing total assets in step 2 itself and doing a calculation of preview withdraw later with updated total assets.Consider calculating shares that need to be burned before updating total assets.
This issue was interesting because Nostra had written tests for passing withdrawals.
Upon further inspection, the Nostra team realised that they missed overriding of total assets from the balance of assets to manually tracked total assets inside shared/erc4626.cairo, and hence, the withdrawal tests passed.
In Cairo, you need to explicitly do so by calling this.*<method>*() unlike Solidity.
This overlooked override could have become a bigger problem moving ahead, if not resolved. Both the incorrect order of operations and the correct total assets override were resolved with this PR and reviewed.
fn max_mint(self: @ContractState, receiver: ContractAddress) -> u256 {
if self.deposit_cap.read() == BoundedInt::max() {
self.max_amount(self.convert_to_shares(self.total_assets()), BoundedInt::max())
} else {
self.convert_to_shares(self.max_deposit(receiver))
}
}
If deposit cap is BoundedInt::max(), you can directly return BoundedInt::max(), instead of doing a max_amount comparison with convert_to_shares(self.total_assets()).
Consider returning BoundedInt::max() directly when deposit cap is BoundedInt::max().
- Nostra doesn't have any slippage protections for ERC4626 actions. The added complexity is not needed for Nostra's current stage of
1 STRK: 1 Share. But if Nostra thinks total assets can become volatile moving forward, Nostra should consider adding those. - Although Nostra is tracking total assets by themselves and also have implemented virtual assets/shares like OZ to nullify empty vault attack vectors, it is still suggested that Nostra seeds the vault with some initial deposit at the time of deployment only.
- Consider checking your upgradability and inheritance thoroughly, particularly from the Cairo and Starknet specific angle.
Curiousapple's review is limited to identifying potential vulnerabilities in the code. It does not investigate security practices, operational security, or evaluate the code relative to a standard or specification.
Curiousapple makes no warranties, either express or implied, regarding the code's merchantability, fitness for a particular purpose, or that it's free from defects.
Curiousapple will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, procurement costs of substitute goods or services, or any claim by any other party.
Curiousapple will not be liable for any consequential, incidental, special, indirect, or exemplary damages, even if it has been advised of the possibility of such damages.
This review does not constitute investment advice, is not an endorsement, and is not a guarantee as to the absolute security of the project.
By deploying or using the code, users agree to use the code at their own risk.
Curiousapple is not responsible for the content or operation of any third-party websites or software linked or referenced in the review, and shall have no liability for the use of such.