# Sona Core Review Log

* Artist Theft via updateReserveAuctionPriceAndCurrency: If an artist has created a sale of an NFT with one currency, say DAI, they can call updateReserveAuctionPriceAndCurrency which checks that 'auction.endingTime < block.timestamp' to see if the auction has a bid yet, but this condition is also true after an auction expires. Therefore an artist can change the currency from DAI to ETH and then call settle to receive far more value (~2k more usd value at time of writing). The assets taken are stolen from other artist's auctions.
* Severity Estimate: Critical [Likelihood: High, Impact: High]
* Remediation Recommendation: Require zero to check uninitialization instead.
* Status: Resolved
* Permissionless Theft of All assets in Auction from a combo of two smaller impact bugs.
* Bug 1: A toxic currency contract can be set and then re-enter during bid function to make changes to the contract while auction.endingTime == 0. This bypasses checks in setter functions that an auction does not have bids while also setting currentBidAmount to an attacker controlled value.
* Bug 2: 'updateReserveAuctionPrice' and 'updateReserveAuctionPriceAndCurrency' do not check that the current reserve price != 0, which lets an attacker bypass the checks that an auction has been initialized without calling 'createReserveAuction'.
* The attack chain is as follows:
* 1) The attacker uses a contract to call 'updateReserveAuctionPriceAndCurrency' for a auction with tokenId = their contract address. [note: 'onlySonaAdminOrApprovedTokenOperator' is true in this case as msg.sender = \_tokenId.getAddress()] They set the currency to their contract address and reservePrice = 1
* 2) The attacker calls bid with an arbitrarily high amount, the bid function calls transferFrom on the attacker contract which then re-enters the auction contract and sets the currency to one with value (ie: eth or dai). After returning it sets the currentBidAmount to the attacker's chosen value.
* 3) The attacker waits for the auction to complete
* 4) The attacker calls settle which will pass as auction reservePrice != 0, enough time has passed, and a bid was set. Settle transfers the value the attacker choose to them.
* Severity Estimate: Critical [Impact: High, Likelihood: High]
* Remediation Recommendation: Fix both bugs, as both may have other exploit paths. Check that the auction was initiated in 'updateReserveAuctionPrice' and 'updateReserveAuctionPriceAndCurrency'. Do the token transfer from the user controlled currency after any effects of the first bid.
* Status: Resolved
* Claiming rewards out of order will cause users to lock rewards: The checks in the sona rewards lib are that a user has start > last end, but if the user waits for two new roots to post and then claims the most recent first their latest will be the top of the rewards list. This prevents claiming the rewards in the previous root.
* Severity: Low [Impact: Medium, Likelihood: Low]
* Recommendation: Enforce sequentiality of the rewards claims
* Status: Resolved
* Burns can be reversed from SonaDirectMint: The signatures are able to be replayed from the mint function if all previous assets are burned. While hard to implement in practice this could create problems as (1) this will reset any changes to the NFT such as new metadata (2) it can cause problems if you ever try to migrate the system or have other onchain contracts which burn the core nfts.
* Severity: Low [Impact: Medium, Likelihood: Low]
* Remediation Recommendation: Mark the token id or metadata as minted and don't delete this field on burns.
* Status: Resolved
* MEV on Chainlink Price Lag: The chainlink price in theory should update on a 0.5% deviation from the current mid market price. Firstly this means that there are many cases where the sona swap will pay an extra 0.5% in slippage to sandwiches. However, this is worse in black swan cases as the crypto market is marked by jump discontinuities in price. Chainlink will update as quickly as possible in this case but you can still face large (> 0.5%) losses vs market price in such a case by ordering your swap before the chainlink update. Because swap transactions could be submitted with low gas or much lower gas than needed they could hang in the mem pool long enough to cause this to be likely.
* Severity: Medium [Impact: Medium, Likelihood: Medium]
* Remediation Recommendations: (1) User swap transactions should be submitted with a high base fee (2) User swap transactions should be submitted to the swap from private endpoints (3) Chainlink prices should not be used and instead users should submit prices.
* Status: Acknowledged
* MEV due to Unusually High Slippage Settings: The average slippage execution has been estimated to be around 7 bps + fees in uniswap's pools for ETH. These are their lowest fee pools and the most liquid. Both the selection of the 0.5% fee pool and the 3% slippage pool puts the fees >10x more than needed. The 0.05% fee pool is the most common usdc eth pool. Sandwich attacks or other MEV manipulations can guarantee you to get a price with 3.5% of fee where you should be getting closer to 0.15% fees even for non optimal executions.
* Severity: Medium [Impact: Medium, Likelihood: High]
* Remediation Recommendations: The default settings should be at minimum increased and should also be possibly configurable instead of defaults. User transactions should be submitted through private relays.
* Status: Resolved
* Burning the Sona Admin Key to Address 0 Lets Anyone be admin: The sona admin library calls ecrecover on the signature and then checks it against the stored admin key value. Ecrecover returns 0 in solidity for a point off the elliptic curve and therefore anyone could create signatures to various effects.
* Severity: Low/Note [Impact: Medium, Likihood: Low]
* Remediation Recommendations: Either don't allow the zero return from ecrecover or initalize the contract to 0xdead or another address known to not have a public key.
* Status: Resolved
* Change Currency front running: If the artist starts an auction with a a currency like say USDC and then front runs the first bid on the auction with a change to a currency such as DAI the user's bid can still succeed if they have set an allowance on the auction contract. This can enable a theft of funds greater than the TVL of the auction contract. Theoretically the loss from this in some setups is unbounded, however that requires very specific setups. A user who is phished (or otherwise socially engineered) may also be particularly vulnerable to this as A phisher could set the reserve price on a new edition to be equal to exactly the allowance or account balance of the target asset, making it highly likely to succeed.
* Severity: High [Impact: High, Likelihood: Medium]
* Remediation Recommendations: Remove the capacity of the artist to change the currency.
* Status: Resolved

Note Levels:
* .send can be susceptible to changes in the gas price, leading to bricking in some cases
* The auction struct is deleted after payouts leading to possible re-entrancy by calling back in from the payout address. TODO - Need gas estimates on this to see how likely it is (not very)
* Some non standard tokens don't return calldata and it would break the transfer Token out function.
* solady is still in audits, should be cautious about using the merkle lib.
* Direct mint can violate the uniqueness of the metadata which is checked by the auction contract.
* Opt - Swap contact can safely do unlimited approval to router at construction and not call approval on each trade.
* The data conversion from the chainlink price feed to eth price quote looses two decimals of accuracy because you divide by 100 when normalizing it to usdc decimals, instead you could wait to remove them after the eth multiplication and it will be slightly more numerical accurate.
* Adding bounds to the fee percents is important because of the underflow in the payout flow.
* The typo in the name of 'mintMulipleToArtist' bugs me