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

Lows

1. Allowance not revoked from previous router after router change

Vulnerability Details

The three strategies grant infinite approval to their respective routers, but in the case of StrategyOp.sol and StrategyArb.sol, these routers can be changed. However, when the routers are changed, the infinite approval is not revoked from the previous router. StrategyMainnet.sol itself can't change the router.
Using StrategyArb.sol as an example, in _initStrategy, max approval is granted to the router.

function _initStrategy() internal {
router = 0xAAA87963EFeB6f7E0a2711F397663105Acb1805e;
@> underlying.safeApprove(address(router), type(uint256).max);
}

And in the setRouter function, the management can change the router. But as can be observed, approval from the previous router is not revoked, and cannot be.

function setRouter(address _router) external onlyManagement {
router = _router;
underlying.safeApprove(router, type(uint256).max);
}

This is very dangerous, because if the router is compromised, it can be used to drain assets from the strategy. Since the router an external integration, outside the control of the protocol, hence the ability to change it, the older router still holds max approval. Even if its changed because of a compromise or any other reason, it still has access to the strategy's assets.

Also, we can observe that the router in use is upgradeable, and currently points to an implementation that has an admin. A potential attack vector here is that if ramses admin gets compromised, the roter can be upgraded to a malicious implementation that can be used to transfer assets from various addresses that had given the router max approval, which will incidentally include our strategy, leading to loss of funds for the protocol. So even if news gets out that the router is now compromised, there's no way to protect the protocol from the compromised router, though setRouter can be used to change router, because the approval is still there.

A fairly similar attack type occured on Socket protocol sometime this year, in which the attacker injected a malicious transferFrom function through which wallets that had given the contract max approvals had their approved assets drained.

The same issue exists in StrategyOp.sol, where router can be changed but previous approval is also not revoked.

function setRouter(address _router) external onlyManagement {
router = _router;
@> underlying.safeApprove(router, type(uint256).max);
}

Recommendations

Zero out the old router's approval before setting a new one.

function setRouter(address _router) external onlyManagement {
+ underlying.safeApprove(router, 0);
router = _router;
underlying.safeApprove(router, type(uint256).max);
}

2. Curve router cannot be updated in StrategyMainnet.sol

Vulnerability Details

In the StrategyMainnet.sol, _initStrategy, is called to approve curve router to spend all of the protcol's tokens. But, in comparison to StrategyArb.sol and StrategyOp.sol, there's no equivalent function to update the curve router address if need be. So if curve router gets compromised, the protocol will not be able to update the router address.

function _initStrategy() internal {
router = ICurveRouterNG(0xF0d4c12A5768D806021F80a262B4d39d26C58b8D);
underlying.safeApprove(address(router), type(uint256).max);
}

Also, similar to l-01 above, the protocol has no way to revoke the router's max approval if it gets compromised.

Recommendations

Introduce an equivalent to setRouter and revoke all approvals before setting new one.

Updates

Appeal created

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

Old router approval is not revoked after an update

Cannot Set A New Router In `StrategyMainnet.sol`

Support

FAQs

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