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

[H-1] Previous Router still have transfer permission

regis

Summary

On the `StrategyArb.sol` and 'StrategyOp.sol' file, the manager has the possibility to change the router address. However, on the setter function, the old router address keep the previous approval. Meaning that in case of an emergency, if an issue is raised on the router, it stills have full access to the fund and can do a 'transferFrom' from the contact.

Vulnerability Details

On the code, the manager have the possibility ot change the router address. However, we do not remove the previous approval from the other contract.

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

Impact

In case of a router exploit, it can have full control of the token owned by the contract and take all the liquidity from it.

Tools Used

Recommendations

I would recommand to first remove the previous approvale before adding the new one to the new router. As an example you could have:

function setRouter(address _router) external onlyManagement {
​
// Remove the previous approval
token.safeApprove(router, 0);
​
// Set the new router
router = _router;
underlying.safeApprove(router, type(uint256).max);
}

Would also recommand to add a testcase scenario for both arbitrum and optimism contract. As:

function testRevokeApprovalOldRouter() public {
// For arbitrum
address router = 0xAAA87963EFeB6f7E0a2711F397663105Acb1805e;
// Check that old router is approved initially
uint256 initialApproval = underlying.allowance(address(transmuter), address(router));
assertEq(initialApproval, type(uint256).max, "Old router should have max approval");
​
address newRouter = address(100);
​
// Change router to newRouter
vm.prank(management);
strategy.setRouter(address(newRouter));
​
// Check that old router approval is revoked
uint256 oldRouterApproval = underlying.allowance(address(strategy), address(router));
assertEq(oldRouterApproval, 0, "Old router approval should be revoked");
​
// Check that new router is approved
uint256 newRouterApproval = underlying.allowance(address(strategy), address(newRouter));
assertEq(newRouterApproval, type(uint256).max, "New router should have max approval");
}
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.