Potlock: Four Findings in Payouts, Refunds, and Fees
Published
Potlock NEAR Rust Smart Contract Security Assessment
Potlock is a funding platform for non-profits and communities, built on NEAR. We found 20 issues across their contracts. This post examines the four critical and high-severity findings: blocked payouts, corrupted payout state, misdirected refunds, and miscalculated fees.
Read the full security report.
GUV-1: DoS of Process Payouts - Critical Severity
Observation
The admin_process_payouts function halts the entire payout process if a single application is unapproved. One bad entry blocks everyone else’s payout.
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
An unapproved application could block the payout batch, preventing approved recipients from receiving their funds.
Proposed Solution
The fix was to change the payout process only to include approved applications.
Takeaway
When processing a batch, test whether one invalid entry can block every valid entry. Make the intended failure behavior explicit.
Remediation
The Potlock team changed the loop to iterate only over approved applications.
Code Snippet
// Iteration now only includes approved applications.
for (project_id, v_app) in self.approved_applications_by_id.iter() {
...
}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);
}
}Impact
Repeated calls could leave payout collections inconsistent, affecting subsequent payouts.
Proposed Solution
Ensure the main and nested collections were completely cleared within chef_set_payouts, using .clear() to prevent state corruption.
Takeaway
Test repeated updates to nested collections, including deletion and reinsertion. Check that related collections remain consistent.
Remediation
The remediation was implemented as follows:
// Proper cleanup of nested collections.
self.payout_ids_by_project_id.clear();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
A Sybil-provider error sent the refund to the contract itself, leaving the donor without the expected refund.
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.
Takeaway
Pass the original caller explicitly when a callback needs to refund them. Check the account receiving the refund in the error-path test.
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.",
);
}GUV-4: Arithmetic Issues In Fee Calculation - High Severity
Observation
We’ve noticed several arithmetic issues in the fee calculation.
Division before multiplication
Integer division by 10,000 discards the remainder before the fee rate is applied. Amounts below 10,000 base units produce a zero fee; other amounts can also be undercharged.
Incorrect rounding direction
The reviewed implementation used the same calculation for protocol, referrer, and chef fees. The proposed fix rounds the protocol fee up and the referrer and chef fees down. These directions reflect this protocol’s fee policy; they are not a universal rule for every financial calculation.
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
The calculation could undercharge fees or charge no fee on small amounts.
Proposed Solution
Multiply before dividing, with appropriate overflow handling, and apply the rounding direction defined for each fee. The example below shows the report’s proposed calculation.
Takeaway
Define the rounding policy for each fee and test small amounts, boundary values, and overflow behavior.
Remediation
Potlock implemented the calculation below, multiplying before division and choosing the rounding direction by fee type.
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)
}Conclusion
Test payout batches with invalid entries, repeated state updates, failed callbacks, and small fee amounts. These cases exposed the four findings described here.
Potlock fixed all of them and let us publish.
Discuss a smart contract security review.
Author
Erick Fernandez - Blockchain Engineer, Founder of Apexwins, and Technical Writer
