Alchemix Transmuter
Alchemix
DeFiFoundrySolidity
16,653 OP
View results
Previous Next
Submission Details
Severity: low
Valid

The `setRouter` call will revert if the strategy has pre-approved the given router.

TheKhans

Summary

When setting the router, we did not revoke the approvals from the current router, which prevents the current router from being set again. This issue is particularly relevant in the context of Arbitrum and Optimism.

Vulnerability Details

Initially, we set the Romses and Velo router addresses. However, if we need to temporarily change the router address or set the router in the strategy contract to the zero address due to a potential hack, and then reset it back to the original address, the transaction will revert. This occurs because the previous approval is not revoked, and a new approval is granted while setting the new router. The safeApproval function enforces that approval can only succeed if there is no pre-existing approval (i.e., the approval is zero).

function setRouter(address _router) external onlyManagement {
router = _router;
underlying.safeApprove(router, type(uint256).max);
}
@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol
function safeApprove(IERC20 token, address spender, uint256 value) internal {
// safeApprove should only be called when setting an initial allowance,
// or when resetting it to zero. To increase and decrease it, use
// 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
require(
(value == 0) || (token.allowance(address(this), spender) == 0),
"SafeERC20: approve from non-zero to non-zero allowance"
);
_callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}

POC :

Add the following test code to Operation.t.sol:

​
function test_changeRouter() public {
console.log(strategy.management());
vm.prank(strategy.management());
strategy.setRouter(address(0x123));
if(block.chainid == 42161){
strategy.setRouter(address(0xAAA87963EFeB6f7E0a2711F397663105Acb1805e));
}else if(block.chainid == 10){
strategy.setRouter(address(0xa062aE8A9c5e11aaA026fc2670B0D65cCc8B2858));
}
}
​

and following test command to make file :

FORK_URL := ${OPTIMISM_RPC_URL}
test :; forge test -vv --mt test_changeRouter --fork-url ${FORK_URL}

Run the test with command : make test

Impact

Once the router address is removed or changed, it cannot be set back to the same router. Additionally, if a hack occurs on one of the previous routers, the assets in the strategy could be at risk.

Tools Used

Manual Review

Recommendations

It is recommended to revoke approval from the current router while calling the setRouter() function.

// apply this change to StrategyArb
diff --git a/src/StrategyArb.sol b/src/StrategyArb.sol
index 574b830..ffc7fa7 100644
--- a/src/StrategyArb.sol
+++ b/src/StrategyArb.sol
​
function setRouter(address _router) external onlyManagement {
- router = _router;
+ if (underlying.allowance(address(this), router) > 0) {
+ underlying.safeApprove(address(router), 0);
+ }
+ router = _router;
underlying.safeApprove(router, type(uint256).max);
}
​
// apply this change to StrategyOp
​
diff --git a/src/StrategyOp.sol b/src/StrategyOp.sol
index 5514039..b5a223a 100644
--- a/src/StrategyOp.sol
+++ b/src/StrategyOp.sol
@@ -46,6 +46,9 @@ contract StrategyOp is BaseStrategy {
NOTE - only used if want to upgrade router
*/
function setRouter(address _router) external onlyManagement {
+ if (underlying.allowance(address(this), router) > 0) {
+ underlying.safeApprove(address(router), 0);
+ }
router = _router;
underlying.safeApprove(router, type(uint256).max);
}
Updates

Appeal created

inallhonesty Lead Judge 10 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.