minhtrng - Cant withdraw pre-maturity

[sherlock-admin]

minhtrng

unlabeled

Cant withdraw pre-maturity

Summary

Withdrawal pre-maturity through ERC5095 does not work because funds arent pulled from user.

Note: everything mentioned below applies to ERC5095.redeem analogously

Vulnerability Detail

Pre-maturity, the function ERC5095.withdraw is supposed to sell iPT tokens of a specified owner on the corresponding pool by using MarketPlace.sellPrincipalToken. The latter will attempt to transfer the iPT tokens to sell from msg.sender (the ERC5095 contract) to the pool:

Safe.transferFrom(
    IERC20(address(pool.fyToken())),
    msg.sender,
    address(pool),
    a
);

In the ERC5095.withdraw function there is currently no functionality that pulls the tokens from the owner to the ERC5095 contract, hence the call to sellPrincipalToken will fail due to insufficient balance. To make this work, a user would be required to atomically send his iPT tokens to the ERC5095 contract with an immediate call of withdraw right after.

The current test case testWithdrawPreMaturity uses a cheatcode to ensure that shares are minted onto the token, but that does not reflect a real world scenario. If there were iPT tokens on the ERC5095 contract, anyone would be able to perform the sell and take the underlying (POC under #Code Snippet)

Impact

Using the ERC5095.withdraw function pre-maturity will always revert, unless there are already iPT tokens on the contract, in which case anyone would be able to sell them for underlying tokens.

Code Snippet

This gist serves as a POC to the claims made under # Vulnerability Detail. The test case testWithdrawPreMaturityNoMagicMint shows the revert due to insufficient balance, if the deal cheatcode is ommited from the testWithdrawPreMaturity test.

The test case testWithdrawPreMaturityDifferentAddress shows that anyone can perform a withdraw of tokens that are sent to ERC5095 contract, which is why there is a need to send the token and perform the sell for underlying tokens atomically.

Tool used

Manual Review

Recommendation

In ERC5095.withdraw, add a Safe.transferFrom call that transfers the tokens in the branches that handle the pre-maturity case.

In the test case testWithdrawPreMaturity, mint the shares to the calling address instead of directly to the token address. Perform an approve and assert that the shares are pulled into the ERC5095 contract when calling its withdraw function.

The problem seems to apply to ERC5095.redeem analogously.

Duplicate of #195

[Minh-Trng]

Escalate for 100 USDC Duplicate of #195 I suspect this might have been closed without further checking because I have not provided a link to the corresponding part of the source code. To my own defense, the bot that checked for correct formatting was down during that period and I was not familiar with the conventions as this is my second sherlock contest. The source code that is affected by the issue is still described unambigously.

[sherlock-admin]

Escalate for 100 USDC Duplicate of #195 I suspect this might have been closed without further checking because I have not provided a link to the corresponding part of the source code. To my own defense, the bot that checked for correct formatting was down during that period and I was not familiar with the conventions as this is my second sherlock contest. The source code that is affected by the issue is still described unambigously.

You've created a valid escalation for 100 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.

[Evert0x]

Escalation accepted

[sherlock-admin]

Escalation accepted

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.