DeFiFoundrySolidity
16,653 OP
View results
Submission Details
Severity: low
Valid

Old Routers Retain Approval of Underlying Funds: A Critical Oversight Leading to Risks of Double Spending, Liquidity Fragmentation, and Fund Loss

Summary

In the StrategyOp and StrategyArb contracts, there is a management-specific function, setRouter, responsible for updating the router address. This router facilitates routing keepers' deposits to optimize yield generation. However, a critical oversight exists: when a new router is set, approvals for the old router are not revoked.

This oversight exposes both strategies to severe vulnerabilities commonly seen in Web3, DeFi, and smart contracts, including unauthorized fund access, double spending, liquidity fragmentation, and fund loss.

The relevant functions are as follows:

StrategyOp::setRouter:

function setRouter(address _router) external onlyManagement {
router = _router;
// @info: missing old router validation check
// @info: approval for previous router not revoked
underlying.safeApprove(router, type(uint256).max);
}

StrategyArb::setRouter:

function setRouter(address _router) external onlyManagement {
router = _router;
// @info: missing old router validation check
// @info: approval for previous router not revoked
underlying.safeApprove(router, type(uint256).max);
}

Although these functions validate inputs through onlyManagement, they fail to revoke approvals for the old router, leading to significant vulnerabilities.

Vulnerability Details

1. Security Risks

  • Unauthorized Access to Funds:
    If the old router retains approval to spend tokens, it can still withdraw funds from the protocol, even if it is no longer in use.

    • Example: If the old router is compromised or has a hidden vulnerability, an attacker can exploit it to drain funds from the protocol.

  • Malicious Exploits:
    If the private keys for the old router are leaked or its smart contract is exploited, malicious actors could use the remaining approval to siphon funds without the protocol’s knowledge.

2. Operational Issues

  • Accidental Transactions:
    Users or automated systems might unintentionally interact with the old router, causing transactions to be routed incorrectly.

    • Impact: Funds may be deposited into deprecated or unintended contracts, leading to inefficiencies or potential loss.

  • Confusion in Asset Management:
    Active approvals for multiple routers can create ambiguity about which router manages which assets, making audits and fund tracking more difficult.

3. Fund Mismanagement

  • Double Spending:
    If both the old and new routers hold active approvals, assets could be inadvertently allocated to both strategies simultaneously. This could result in inconsistencies in the protocol's accounting and yield calculations.

  • Liquidity Fragmentation:
    Tokens could remain locked in the old router if it still holds authorization but is no longer actively managed. This leads to reduced protocol efficiency and fragmentation of liquidity.

Impact

The failure to revoke old router approvals can result in:

  1. Token Theft: Funds could be drained by malicious actors exploiting compromised routers.

  2. Operational Failures: Incorrect or unintended routing of funds, causing disruptions in strategy execution.

  3. Double Spending: Assets might be spent or allocated redundantly, compromising accounting integrity.

  4. Liquidity Inefficiency: Tokens might remain stuck in obsolete routers, reducing capital efficiency.

This oversight significantly increases the security risk and operational fragility of the protocol.

Tools Used

Manual Review

Recommendations

To address this vulnerability, revoke approvals for the old router before assigning a new one. This ensures that the old router no longer has the authority to spend the protocol's tokens.

Updated setRouter Function:

function setRouter(address _router) external onlyManagement {
+ // Revoke approval for the old router
+ if (router != address(0)) {
+ underlying.safeApprove(router, 0);
+ }
// Set new router and approve it
router = _router;
underlying.safeApprove(router, type(uint256).max);
}

Benefits of This Change:

  1. Prevents Unauthorized Access: The old router loses all approvals, eliminating risks of misuse or exploits.

  2. Improves Security Hygiene: Minimizes the attack surface and mitigates potential fund mismanagement.

  3. Ensures Operational Integrity: Ensures only the new router has the ability to manage tokens.

By implementing this safeguard, the protocol can significantly enhance its resilience and protect user funds from avoidable vulnerabilities.

Updates

Appeal created

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Old router approval is not revoked after an update

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Old router approval is not revoked after an update

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.