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

Blocking of BPT Locking Via cooldown #341

Closed
code423n4 opened this issue May 25, 2022 · 3 comments
Closed

Blocking of BPT Locking Via cooldown #341

code423n4 opened this issue May 25, 2022 · 3 comments
Labels
bug Something isn't working invalid This doesn't seem right resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/CrvDepositor.sol#L80

Vulnerability details

Issue: A malicious action by daoOperator can call setCooldown, blocking the ability to lock "Curve". daoOperator also has the ability to set a new daoOperator address, which would make the effects permanent.

Consequences: Permanently inoperable smart contract.

Mitigations:

Solution 1:

  • Consider removing the cooldown functionality.
  • If cooldown is required for business logic, ensure that the daoOperator is not vulnerable to governance attack.
  • Do not allow daoOperator to set a new address.

Solution 2:

  • The cooldown could be an upper bounded timestamp instead (so that cooldowns can't be forever, by definition of a cooldown)
  • daoOperator should be a multisig wallet with a timelock
@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 May 25, 2022
code423n4 added a commit that referenced this issue May 25, 2022
@0xMaharishi 0xMaharishi added invalid This doesn't seem right sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels May 28, 2022
@0xMaharishi
Copy link

This is intentional to allow potential migration to new veToken if its ever created

@0xMaharishi 0xMaharishi added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label May 30, 2022
@0xMaharishi
Copy link

@dmvt
Copy link
Collaborator

dmvt commented Jun 20, 2022

This is invalid. The new daoOperator would have the ability to change the value at will. I've said this in other contests, but will say it again here: If we assume the DAO / owner to be a bad actor, the entire protocol is compromised. To mitigate this is to remove the DAO / admin and all associated functionality, which brings with it a net harm to users. Certain protocols, including this one, need to be able to upgrade so that they can continue to function as the rest of the world changes around them. This functionality is by design.

@dmvt dmvt closed this as completed Jun 20, 2022
@dmvt dmvt removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants