Security Insights Series - Potlock
Potlock NEAR Rust Smart Contract Security Assessment
Welcome back to our Security Insights Series, where we look into major vulnerabilities within some of the projects Guvenkaya has reviewed in the Web3 space. Today, we’re focusing on the Potlock NEAR Rust Smart Contract Security Assessment.
Potlock uses blockchain technology to change how non-profits and communities raise funds, aiming to be transparent and efficient. However, with innovations, strong security is essential. This blog post highlights the main vulnerabilities that could seriously threaten Potlock’s systems and shows how solving these problems strengthens not only Potlock but also security practices throughout the DeFi world.
Socials
Before we start, consider following us and Potlock Protocol on Social Media:
Summary: Major Vulnerabilities - Focusing on Critical and High Severity Issues
In our evaluation, we found 20 vulnerabilities of varying severity. This analysis highlights the critical and high-severity issues that need quick action.
We use a clear structure to review each problem: Observe to identify the issue, assess its Impact, and then propose a Solution and Remediation steps.
This method helps new auditors and innovative projects understand and enhance security measures.
GUV-1: DoS of Process Payouts - Critical Severity
Observation:
A serious flaw was found in the admin_process_payouts function during our review of Potlock’s smart contracts. This crucial function stops the payout process if even one application is not approved by the system. It opens up doors for a Denial of Service.
The code snippet below shows the problematic logic:
// pot:admin_process_payouts:pot/src/payout.rs
for (project_id, v_app) in self.applications_by_id.iter() {
// TODO: update this to only go through approved applications
// mapping
self.assert_approved_application(&project_id); // TODO: check that the project is not owner, admin or chef
let application = Application::from(v_app);
// ...if there are payouts for the project...
if let Some(payout_ids_for_project) = self.payout_ids_by_project_id.get(&project_id)
{
// TODO: handle milestones (for now just paying out all payouts)
for payout_id in payout_ids_for_project.iter() {
let payout = Payout::from(self.payouts_by_id.get(&payout_id).expect("no payout"));
if payout.paid_at.is_none() {
// ...transfer funds...
Promise::new(application.project_id.clone())
.transfer(payout.amount.0)
.then(
Self::ext(env::current_account_id())
.with_static_gas(XCC_GAS)
.transfer_payout_callback(payout));
}
}
}
}
self.all_paid_out = true;A proof of concept in the report showed how a malicious actor could use this flaw to stop the payout process, effectively causing a denial of service.
Impact:
This vulnerability opened Potlock to a denial of service (DoS) attack, where an unapproved application could be inserted to block all other payouts. Such an attack would harm the platform’s smooth running and weaken user trust. The severity of this issue posed major risks to Potlock’s mission of enabling transparent fundraising.
Proposed Solution
The fix was to change the payout process only to include approved applications.
Insights for Projects
When looping through elements, ensure that one invalid condition doesn’t stop the process for other elements or assert whether that’s the intended behaviour.
Insights for Security Engineers
Security engineers should carefully check loop logic. Consider if a single element’s failure should cause others to fail and if this is what you want.
Remediation
The Potlock team quickly implemented this fix.
Code Snippet
// Iteration now only includes approved applications.
for (project_id, v_app) in self.approved_applications_by_id.iter() {
...
}The Potlock team fixed the problem by changing the loop logic only to consider approved applications, protecting the platform from potential Denial of Service.
GUV-2: State Corruption On Setting Payout - Critical Severity
Observation:
We identified a second critical flaw in the chef_set_payouts function. This function sets application payouts but fails to completely clear the payout_ids_by_project_id nested collection, leaving an empty UnorderedSet that could cause the collection to become inconsistent with further calls to chef_set_payouts.
The code snippet illustrates this flaw:
// pot:chef_set_payouts:pot/src/payout.rs
for application_id in self.approved_application_ids.iter() {
if let Some(payout_ids_for_application) = self.payout_ids_by_project_id.get(&application_id) {
for payout_id in payout_ids_for_application.iter() {
self.payouts_by_id.remove(&payout_id);
}
self.payout_ids_by_project_id.remove(&application_id);
}
}This issue was especially worrying because it could corrupt the payout state, leading to mistakes in how payouts were given out—a critical issue for any financial platform.
Impact:
This vulnerability threatened the reliability of payout data, possibly causing incorrect or missed payouts. This could lower the platform’s reliability and trust, severely impacting its stable operation and user confidence.
Proposed Solution:
Ensure the main and nested collections were completely cleared within chef_set_payouts, using .clear() to prevent state corruption.
Insights for Projects
Projects must ensure that the data is added/removed correctly from the nested collections to prevent state corruption.
Insights for Security Engineers
Security engineers should double-check how nested collections are managed for adding and removing entries.
Remediation
The remediation was implemented as follows:
// Proper cleanup of nested collections.
self.payout_ids_by_project_id.clear();By implementing this change, Potlock fixed the vulnerability, reinforced the integrity of the payout mechanism, and protected the platform from potential state corruption issues.
GUV-3: User Funds Not Returned On Error From Sybil Provider - High Severity
Observation:
During the Potlock smart contract audit, a major vulnerability was spotted concerning how user funds were handled if there was an error from the Sybil provider. The callback parameters were set up incorrectly, leading to funds being sent to the contract instead of being refunded to the user.
Code Snippet
// Original flawed callback implementation
#[private]
pub fn sybil_callback(
&mut self,
deposit: Balance,
project_id: Option<ProjectId>,
message: Option<String>,
referrer_id: Option<AccountId>,
matching_pool: bool,
#[callback_result] call_result: Result<bool, PromiseError>
) -> Promise {
let caller_id = env::predecessor_account_id();
if call_result.is_err() {
// Funds are mistakenly sent to the contract itself
Promise::new(caller_id).transfer(deposit);
}
}Impact:
This issue was a high risk to user assets, where mistakes in the Sybil verification process would not result in the expected refund, leading to possible financial losses. This could damage user trust and the platform’s reputation by mismanaging user funds.
Proposed Solution:
The solution was to correct the callback parameter handling to ensure that the funds would be accurately returned to the user’s account in case of a Sybil provider error rather than being wrongly sent to the contract.
Insights for Projects:
It’s crucial for projects to ensure that callback functions in smart contracts are implemented correctly. Always check that the correct caller is passed through callbacks, and remember that the predecessor in the callback is the contract that calls that callback.
Insights for Security Engineers:
Security engineers must thoroughly test smart contract callbacks to protect against misrouted funds. Double-check whether the correct caller is passed in the callback
Remediation:
The fix involved adjusting the callback function to properly return funds to the user in case of a Sybil provider error by passing the correct caller ID.
Code Snippet
// Corrected callback implementation
#[private]
pub fn sybil_callback(
&mut self,
caller_id: AccountId,
deposit: Balance,
project_id: Option<ProjectId>,
message: Option<String>,
referrer_id: Option<AccountId>,
matching_pool: bool,
#[callback_result] call_result: Result<bool, PromiseError>,
) -> Promise {
if call_result.is_err() {
log!(format!(
"Error verifying sybil check; returning donation {} to donor {}",
deposit, caller_id
));
Promise::new(caller_id).transfer(deposit);
env::panic_str(
"There was an error querying sybil check. Donation has been returned to donor.",
);
}
By fixing the callback function, Potlock ensured the secure handling of user funds, aligning with best practices for smart contract development and reinforcing the platform’s commitment to user security and trust.
GUV-4: Arithmetic Issues In Fee Calculation - High Severity
Observation
We’ve noticed several arithmetic issues in the fee calculation.
Division before multiplication
Dividing before the multiplication can cause errors or even wrong results. The fee will always be zero if the amount is smaller than the total basis points. This could allow users to completely avoid the fee when the amount sent is below this threshold. If the amount equals the total basis points, the fee will always be the basis points, which might substantially reduce the fees for users when the amount sent is exactly this threshold. The fee isn’t calculated properly if the amount exceeds the total basis points.
Incorrect rounding direction
The protocol should always be rounded in a way that doesn’t favour the user. Therefore, the division should be rounded down, allowing the user to receive a lower fee. While the same function calculates the protocol, referrer, and chef fees, the referrer and chef fees should also be rounded down. However, the protocol fee should be rounded up.
Code Snippet
// Flawed fee calculation method
pub(crate) fn calculate_fee(
amount: u128,
basis_points: u32
) -> u128 {
let total_basis_points = 10_000u128;
let fee = (amount / total_basis_points) * basis_points as u128; // Division before multiplication can lead to precision loss
fee
}Impact
Mistakes in calculating fees threaten Potlock’s financial structure. Wrong fee amounts could reduce the platform’s income and open up to potential exploits. If not fixed, these problems could damage user trust and risk the platform’s future.
Proposed Solution
To fix these errors in fee calculation, we suggest a method that multiplies before dividing and includes rounding that favours the platform. This method reduces mistakes and ensures the fees match the platform’s intent.
Insights for Projects
Projects, especially in the DeFi space, need to carefully check and confirm the accuracy of financial calculations in their smart contracts. Accurate fee calculations are vital for keeping the platform economically sound and maintaining user trust. Always ensure you round against the user and perform multiplication before division.
Insights for Security Engineers
Security engineers should closely examine the math in smart contracts, especially for financial transactions. Catching and fixing these issues early can prevent big problems for the platform’s financial setup. Check that calculations are done in the right order and consider which way rounding should go.
Remediation
Potlock has put in place a better way to calculate fees that fixes the order of operations and includes the right rounding methods.
Code Snippet
fn calculate_fee(amount: u128, basis_points: u32, is_protocol: bool) -> u128 {
let total_basis_points = 10_000u128;
let amount = basis_points as u128 * amount;
if !is_protocol {
return amount / total_basis_points
}
amount.div_ceil(total_basis_points)
}Potlock has dealt with the risk of errors by improving how fees are calculated, protecting its financial health and ensuring a fair and clear fee system for its users.
Conclusion
This dive into Potlock’s smart contract vulnerabilities and how they were fixed shows a key lesson about blockchain and DeFi: innovation must go hand in hand with thorough security. We commend Potlock for quickly addressing each issue we found and demonstrating their strong commitment to security.
Moving forward, we aim to explain the complexities of smart contract security, providing insights and advice to developers and security experts. Your involvement and feedback are crucial as we tackle these challenges together, striving for a safer and more open DeFi future.
If you’re developing a project and want Guvenkaya to conduct a security review, you can:
- Get a quote at: Guvenkaya Website
- Contact us on: Timur Guvenkaya Telegram
- Email us at: contact@guvenkaya.co
Author
Erick Fernandez - Blockchain Engineer, Founder of Apexwins, and Technical Writer