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.

Sign up for GitHub

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

Jump to bottom

QA Report #44

Open
code423n4 opened this issue Jun 12, 2022 · 4 comments
Open

QA Report #44

code423n4 opened this issue Jun 12, 2022 · 4 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

code423n4 commented Jun 12, 2022 •
edited by 0xleastwood
Loading

Possible to set RouterOwner to 0 without proposal period

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/RoutersFacet.sol#L434

Vulnerability details

Impact

In RoutersFacet.sol#L434, it is checked if the proposal period has passed. However, it is possible for the owner to set a new owner without waiting for the delay. When proposedRouterTimestamp and proposedRouterOwners are 0 for a given router (i.e., uninitialized), the owner can call acceptProposedRouterOwner (because onlyProposedRouterOwner accepts calls by the owner in such a scenario) and immediately set the owner to 0.

Recommended Mitigation Steps

Check proposedRouterTimestamp is 0.
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jun 12, 2022
code423n4 added a commit that referenced this issue Jun 12, 2022
@code423n4
Lambda issue #44
ca13959
@jakekidd jakekidd added disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels Jun 25, 2022
@jakekidd
Copy link
Collaborator

  • onlyProposedRouterOwner can call this method, so unless you are the owner or somehow control address(0), it will revert.
  • Routers are managed by their operators. The proposal window is intended to delay transfers of ownership from one EOA to another.
  • The described issue basically just amounts to the owner being able to revoke their ownership instantly. I'm going to leave this as acknowledged because it's technically true, but doesn't create any sort of attack or griefing vector.

@0xleastwood
Copy link
Collaborator

Agree with sponsor. Routers are able to renounce ownership instantly, but this doesn't impact protocol availability nor lock funds or leak value. The router is still able to withdraw to their chosen recipient address as per usual. Downgrading to QA.

@0xleastwood 0xleastwood added duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) labels Aug 15, 2022
@0xleastwood
Copy link
Collaborator

Merging with #43.

@0xleastwood
Copy link
Collaborator

Converting to QA report because #43 is invalid.

@0xleastwood 0xleastwood reopened this Aug 16, 2022
@0xleastwood 0xleastwood removed the duplicate This issue or pull request already exists label Aug 16, 2022
@0xleastwood 0xleastwood changed the title Possible to set RouterOwner to 0 without proposal period QA Report Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jakekidd @0xleastwood @code423n4