hyh - There are no Illuminate PT transfers from the owner in ERC5095's withdraw and redeem before maturity
Opened this issue · 6 comments
hyh
high
There are no Illuminate PT transfers from the owner in ERC5095's withdraw and redeem before maturity
Summary
Illuminate PT's withdraw() and redeem() sell the PTs via pool and return the obtained underlying to the receiver r, but do not transfer these shares from the owner, i.e. selling PTs from the contract balance, if there is any. There is no accounting for the owner side that something was sold on his behalf, i.e. any owner can sell all Pts from the contract balance without spending anything.
Normal operation, on the other hand, is inaccessible, if there are not enough PTs on the Illuminate PT contract balance, withdraw and redeem will be reverting, i.e. an owner is not able to provide anything.
Vulnerability Detail
IMarketPlace(marketplace).sellPrincipalToken transfers the PT to be sold from Illuminate PT contract via Marketplace's Safe.transferFrom( IERC20(address(pool.fyToken())), msg.sender, address(pool),a), but these aren't owner's PTs as nothing was transferred from the owner before and the owner's ERC5095 record aren't updated anyhow.
I.e. o == msg.sender check does virtually nothing as o record neither checked nor changed as a result of withdraw() and redeem(). Say o might not own anything at all at this Illuminate PT contract, the calls succeed anyway as long as the contract has PTs on the balance.
Impact
Anyone can empty the total holdings of Illuminate PTs of the ERC5095 contract by calling withdraw() or redeem(). This is fund stealing impact for the PTs on the balance.
Valid withdraw() or redeem() from real owners will be reverted as long as there will not be enough Illuminate PTs on the balance, i.e. withdrawal before maturity functionality will not be available at all. Although it can be done directly via Marketplace, there also are downstream systems integrated specifically with Illuminate PT contract and execution delays overall do have monetary costs.
Setting the severity to be high as this is the violation of core business logic with total impact from temporal fund freezing to fund loss.
Code Snippet
withdraw() sells the PTs held by Illuminate PT contract, if any, reverting if there are not enough funds on the balance, not transferring PTs from the owner:
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L204-L249
/// @notice At or after maturity, Burns `shares` from `owner` and sends exactly `assets` of underlying tokens to `receiver`. Before maturity, sends `assets` by selling shares of PT on a YieldSpace AMM.
/// @param a The amount of underlying tokens withdrawn
/// @param r The receiver of the underlying tokens being withdrawn
/// @param o The owner of the underlying tokens
/// @return uint256 The amount of principal tokens burnt by the withdrawal
function withdraw(
uint256 a,
address r,
address o
) external override returns (uint256) {
// Pre maturity
if (block.timestamp < maturity) {
uint128 shares = Cast.u128(previewWithdraw(a));
// If owner is the sender, sell PT without allowance check
if (o == msg.sender) {
uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
underlying,
maturity,
shares,
Cast.u128(a - (a / 100))
);
Safe.transfer(IERC20(underlying), r, returned);
return returned;
// Else, sell PT with allowance check
} else {
uint256 allowance = _allowance[o][msg.sender];
if (allowance < shares) {
revert Exception(
20,
allowance,
shares,
address(0),
address(0)
);
}
_allowance[o][msg.sender] = allowance - shares;
uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
underlying,
maturity,
Cast.u128(shares),
Cast.u128(a - (a / 100))
);
Safe.transfer(IERC20(underlying), r, returned);
return returned;
}
}redeem() sells the PTs held by Illuminate PT contract, if any:
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L279-L319
/// @notice At or after maturity, burns exactly `shares` of Principal Tokens from `owner` and sends `assets` of underlying tokens to `receiver`. Before maturity, sends `assets` by selling `shares` of PT on a YieldSpace AMM.
/// @param s The number of shares to be burned in exchange for the underlying asset
/// @param r The receiver of the underlying tokens being withdrawn
/// @param o Address of the owner of the shares being burned
/// @return uint256 The amount of underlying tokens distributed by the redemption
function redeem(
uint256 s,
address r,
address o
) external override returns (uint256) {
// Pre-maturity
if (block.timestamp < maturity) {
uint128 assets = Cast.u128(previewRedeem(s));
// If owner is the sender, sell PT without allowance check
if (o == msg.sender) {
uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
underlying,
maturity,
Cast.u128(s),
assets - (assets / 100)
);
Safe.transfer(IERC20(underlying), r, returned);
return returned;
// Else, sell PT with allowance check
} else {
uint256 allowance = _allowance[o][msg.sender];
if (allowance < s) {
revert Exception(20, allowance, s, address(0), address(0));
}
_allowance[o][msg.sender] = allowance - s;
uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
underlying,
maturity,
Cast.u128(s),
assets - (assets / 100)
);
Safe.transfer(IERC20(underlying), r, returned);
return returned;
}
// Post-maturity
} else {sellPrincipalToken() both functions above invoke transfers IERC20(address(pool.fyToken())) from msg.sender, which is the calling Illuminate PT contract:
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Marketplace.sol#L279-L314
/// @notice sells the PT for the underlying via the pool
/// @param u address of an underlying asset
/// @param m maturity (timestamp) of the market
/// @param a amount of PTs to sell
/// @param s slippage cap, minimum amount of underlying that must be received
/// @return uint128 amount of underlying bought
function sellPrincipalToken(
address u,
uint256 m,
uint128 a,
uint128 s
) external returns (uint128) {
// Get the pool for the market
IPool pool = IPool(pools[u][m]);
// Preview amount of underlying received by selling `a` PTs
uint256 expected = pool.sellFYTokenPreview(a);
if (expected < s) {
revert Exception(16, expected, s, address(0), address(0));
}
// Transfer the principal tokens to the pool
Safe.transferFrom(
IERC20(address(pool.fyToken())),
msg.sender,
address(pool),
a
);
// Execute the swap
uint128 received = pool.sellFYToken(msg.sender, uint128(expected));
emit Swap(u, m, address(pool.fyToken()), u, received, a, msg.sender);
return received;
}Notice that after maturity there is no need to transfer, and the burning is correctly performed by authRedeem():
For example redeem() calls authRedeem():
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L318-L344
// Post-maturity
} else {
if (o == msg.sender) {
return
IRedeemer(redeemer).authRedeem(
underlying,
maturity,
msg.sender,
r,
s
);
} else {
uint256 allowance = _allowance[o][msg.sender];
if (allowance < s) {
revert Exception(20, allowance, s, address(0), address(0));
}
_allowance[o][msg.sender] = allowance - s;
return
IRedeemer(redeemer).authRedeem(
underlying,
maturity,
o,
r,
s
);
}
}authRedeem() burns the shares from the owner f:
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L443-L470
function authRedeem(
address u,
uint256 m,
address f,
address t,
uint256 a
)
external
authorized(IMarketPlace(marketPlace).token(u, m, 0))
returns (uint256)
{
// Get the principal token for the given market
IERC5095 pt = IERC5095(IMarketPlace(marketPlace).token(u, m, 0));
// Make sure the market has matured
uint256 maturity = pt.maturity();
if (block.timestamp < maturity) {
revert Exception(7, maturity, 0, address(0), address(0));
}
// Calculate the amount redeemed
uint256 redeemed = (a * holdings[u][m]) / pt.totalSupply();
// Update holdings of underlying
holdings[u][m] = holdings[u][m] - redeemed;
// Burn the user's principal tokens
pt.authBurn(f, a);Tool used
Manual Review
Recommendation
Consider performing PT transfer before selling them via Marketplace, for example:
withdraw():
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L204-L249
/// @notice At or after maturity, Burns `shares` from `owner` and sends exactly `assets` of underlying tokens to `receiver`. Before maturity, sends `assets` by selling shares of PT on a YieldSpace AMM.
/// @param a The amount of underlying tokens withdrawn
/// @param r The receiver of the underlying tokens being withdrawn
/// @param o The owner of the underlying tokens
/// @return uint256 The amount of principal tokens burnt by the withdrawal
function withdraw(
uint256 a,
address r,
address o
) external override returns (uint256) {
// Pre maturity
if (block.timestamp < maturity) {
uint128 shares = Cast.u128(previewWithdraw(a));
+ _transfer(o, address(this), shares);
// If owner is the sender, sell PT without allowance check
if (o == msg.sender) {
uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
underlying,
maturity,
shares,
Cast.u128(a - (a / 100))
);
Safe.transfer(IERC20(underlying), r, returned);
return returned;
// Else, sell PT with allowance check
} else {
uint256 allowance = _allowance[o][msg.sender];
if (allowance < shares) {
revert Exception(
20,
allowance,
shares,
address(0),
address(0)
);
}
_allowance[o][msg.sender] = allowance - shares;
uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
underlying,
maturity,
Cast.u128(shares),
Cast.u128(a - (a / 100))
);
Safe.transfer(IERC20(underlying), r, returned);
return returned;
}
}redeem():
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L279-L319
/// @notice At or after maturity, burns exactly `shares` of Principal Tokens from `owner` and sends `assets` of underlying tokens to `receiver`. Before maturity, sends `assets` by selling `shares` of PT on a YieldSpace AMM.
/// @param s The number of shares to be burned in exchange for the underlying asset
/// @param r The receiver of the underlying tokens being withdrawn
/// @param o Address of the owner of the shares being burned
/// @return uint256 The amount of underlying tokens distributed by the redemption
function redeem(
uint256 s,
address r,
address o
) external override returns (uint256) {
// Pre-maturity
if (block.timestamp < maturity) {
+ _transfer(o, address(this), s);
uint128 assets = Cast.u128(previewRedeem(s));
// If owner is the sender, sell PT without allowance check
if (o == msg.sender) {
uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
underlying,
maturity,
Cast.u128(s),
assets - (assets / 100)
);
Safe.transfer(IERC20(underlying), r, returned);
return returned;
// Else, sell PT with allowance check
} else {
uint256 allowance = _allowance[o][msg.sender];
if (allowance < s) {
revert Exception(20, allowance, s, address(0), address(0));
}
_allowance[o][msg.sender] = allowance - s;
uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
underlying,
maturity,
Cast.u128(s),
assets - (assets / 100)
);
Safe.transfer(IERC20(underlying), r, returned);
return returned;
}
// Post-maturity
} else {@sourabhmarathe I would like to know why you disagree with severity, seems like a valid high to me.
I believe the ERC5095 doesn't normally hold any balance. The tests in #71 use deal() to give it a balance, which wouldn't happen in reality. The bug here looks to be that withdraw() will fail before maturity since MarketPlace.sellPrincipalToken() won't have any balance to pull out of the ERC5095. As the submitter mentions, MarketPlace can be called by the holder directly in order to sell before maturity, so the function is broken, but there's a workaround. @sourabhmarathe please correct me if I'm mistaken
Originally, I felt that this did not put user funds at risk but certainly agreed with the report (I believe we have a fix for it). However, it seems like this is a serious enough problem where we can keep the severity level High. I removed the dispute label.
Escalate for 10 USDC
Don't agree with high severity. ERC5095 normally doesn't hold any balance so any balance in the contract to be "stolen" was accidentally sent there. Agreed that withdraw/redeem won't work pre-maturity but given that it is basically just a wrapper on Marketplace#sellPrincipleToken, users can just use that instead. Medium seems more appropriate given that users have alternatives to sell pre-maturity and no funds are at risk
Escalate for 10 USDC
Don't agree with high severity. ERC5095 normally doesn't hold any balance so any balance in the contract to be "stolen" was accidentally sent there. Agreed that withdraw/redeem won't work pre-maturity but given that it is basically just a wrapper on Marketplace#sellPrincipleToken, users can just use that instead. Medium seems more appropriate given that users have alternatives to sell pre-maturity and no funds are at risk
You've created a valid escalation for 10 USDC!
To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.