Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit Fixes 1: XION Authentication fix and test #546

Merged
merged 9 commits into from
Dec 6, 2024

Conversation

Kayanski
Copy link
Contributor

@Kayanski Kayanski commented Nov 26, 2024

This Pr aims at adding Auth permissions fix for xion (found during the audit by us)

Changes :

  • Account contract is now always whitelisted, it can execute on itself non-admin actions
  • ownership::assert_nested_owner flag, now checks that the contract has admin execution rights in case of a XION Abstract Account
  • Removed admin checks for xion in the whitelist function (not used anymore)
  • Added a test to make sure it's not possible to bypass the admin check when calling from a module
  • Added admin checks on the Auth Method admin methods (original audit issue)

Checklist

  • CI is green.
  • Changelog updated.

Copy link

cloudflare-workers-and-pages bot commented Nov 26, 2024

Deploying abstract-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 53a1373
Status: ✅  Deploy successful!
Preview URL: https://1d3cd94d.abstract-docs.pages.dev
Branch Preview URL: https://fix-auth-permissions-xio.abstract-docs.pages.dev

View logs

@adairrr adairrr changed the title Added fix and test XION Authentication fix and test Nov 26, 2024
@Kayanski Kayanski marked this pull request as ready for review November 27, 2024 09:56
@Kayanski Kayanski changed the title XION Authentication fix and test Audit Fixes 1: XION Authentication fix and test Nov 27, 2024
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 9 lines in your changes missing coverage. Please review.

Please upload report for BASE (audit-fixes/root@a647229). Learn more about missing BASE report.

Files with missing lines Patch % Lines
framework/contracts/account/src/execution.rs 82.6% 4 Missing ⚠️
.../abstract-std/src/objects/ownership/gov_ownable.rs 81.2% 3 Missing ⚠️
framework/contracts/account/src/contract.rs 66.6% 2 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
framework/contracts/account/src/modules.rs 97.0% <ø> (ø)
...amework/contracts/account/src/modules/migration.rs 95.7% <ø> (ø)
framework/contracts/account/src/contract.rs 85.4% <66.6%> (ø)
.../abstract-std/src/objects/ownership/gov_ownable.rs 92.7% <81.2%> (ø)
framework/contracts/account/src/execution.rs 85.8% <82.6%> (ø)

@Kayanski Kayanski changed the base branch from main to audit-fixes/root November 27, 2024 12:12
@Kayanski Kayanski requested a review from adairrr November 27, 2024 14:23
framework/contracts/account/src/execution.rs Show resolved Hide resolved
framework/contracts/account/tests/xion.rs Show resolved Hide resolved
if let Ok(true) = AUTH_ADMIN.query(querier, address) {
return Ok(());
} else {
return Err(crate::objects::ownership::GovOwnershipError::NotOwner);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return a more specific error such as NotAdmin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really because this logic ensures that it's the owner calling the contract. If a module calls the account, then it's not the owner calling it. So I would stick with that

@Kayanski Kayanski merged commit d538013 into audit-fixes/root Dec 6, 2024
11 of 14 checks passed
@Kayanski Kayanski deleted the fix/auth-permissions-xio branch December 6, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants