Users will lose all of their money during pool migration

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/zaps/PoolMigrationZap.sol#L59 https://github.com/code-423n4/2022-05-backd/blob/1136e0cdc8579614a33832fe2a21785d60aac19b/protocol/contracts/pool/LiquidityPool.sol#L527-L559

Vulnerability details

Impact

Users will lose all of their money when they migrate by calling PoolMigrationZap.migrate()

Proof of Concept

File: protocol/contracts/zaps/PoolMigrationZap.sol   #1

52       function migrate(address oldPoolAddress_) public override {
53           ILiquidityPool oldPool_ = ILiquidityPool(oldPoolAddress_);
54           IERC20 lpToken_ = IERC20(oldPool_.getLpToken());
55           uint256 lpTokenAmount_ = lpToken_.balanceOf(msg.sender);
56           require(lpTokenAmount_ != 0, "No LP Tokens");
57           require(oldPool_.getWithdrawalFee(msg.sender, lpTokenAmount_) == 0, "withdrawal fee not 0");
58           lpToken_.safeTransferFrom(msg.sender, address(this), lpTokenAmount_);
59           uint256 underlyingAmount_ = oldPool_.redeem(lpTokenAmount_);

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/zaps/PoolMigrationZap.sol#L52-L59

The redeem() call above on line 59 calls redeem(<amount>, 0) where zero indicates the minimum amount to receieve from the redemption:

File: protocol/contracts/pool/LiquidityPool.sol   #2

527       /**
528        * @notice Redeems the underlying asset by burning LP tokens.
529        * @param redeemLpTokens Number of tokens to burn for redeeming the underlying.
530        * @param minRedeemAmount Minimum amount of underlying that should be received.
531        * @return Actual amount of the underlying redeemed.
532        */
533       function redeem(uint256 redeemLpTokens, uint256 minRedeemAmount)
534           public
535           override
536           returns (uint256)
537       {
538           require(redeemLpTokens > 0, Error.INVALID_AMOUNT);
539           ILpToken lpToken_ = lpToken;
540           require(lpToken_.balanceOf(msg.sender) >= redeemLpTokens, Error.INSUFFICIENT_BALANCE);
541   
542           uint256 withdrawalFee = addressProvider.isAction(msg.sender)
543               ? 0
544               : getWithdrawalFee(msg.sender, redeemLpTokens);
545           uint256 redeemMinusFees = redeemLpTokens - withdrawalFee;
546           // Pay no fees on the last withdrawal (avoid locking funds in the pool)
547           if (redeemLpTokens == lpToken_.totalSupply()) {
548               redeemMinusFees = redeemLpTokens;
549           }
550           uint256 redeemUnderlying = redeemMinusFees.scaledMul(exchangeRate());
551           require(redeemUnderlying >= minRedeemAmount, Error.NOT_ENOUGH_FUNDS_WITHDRAWN);
552   
553           _rebalanceVault(redeemUnderlying);
554   
555           lpToken_.burn(msg.sender, redeemLpTokens);
556           _doTransferOut(payable(msg.sender), redeemUnderlying);
557           emit Redeem(msg.sender, redeemUnderlying, redeemLpTokens);
558           return redeemUnderlying;
559       }

https://github.com/code-423n4/2022-05-backd/blob/1136e0cdc8579614a33832fe2a21785d60aac19b/protocol/contracts/pool/LiquidityPool.sol#L527-L559

Because the minRedeemAmount is set to zero, users will be vulnerable to sandwich attacks, and will lose all of their money.

The mechanics of the attack are:

• Attacker sees a user's migration transaction in the mempool
• Attacker creates a transaction to take out a flash loan and use the money to drive down the value of the token the user is redeeming
• Attacker creates another transaction to change things back to the way they were by buying at the projected cheaper price
• Attacker mines a block with his/her first transaction, followed by the user's migration transaction, followed by the attacker's second transaction

The attacker pockets the difference between the amount of tokens the user should have been able to redeem, and the actual amount they were able to redeem.

Tools Used

Code inspection

Recommended Mitigation Steps

Supply the original amount, with some small percentage removed for slippage, instead of zero

[chase-manning]

There is no sandwich attack opportunity as the amount received only varies based on the pool exchange rate. And the pool exchange rate cannot be manipulated by an attacker.

[GalloDaSballo]

Given the consideration that the > hypothetical < loss would be through front-running, High Severity would be out of the question.

Per the sponsor reply I went and checked redeem and how exchangeRate is used, and have to agree that beside getWithdrawalFee there is no obvious way in which value would be lost through a migration.

Because the finding is contingent on demonstrating a change in exchangeRate that would be:

• Unfavourable to the caller
• Frontrunnable in a MEVable way

In lack of a POC, I can't help but mark the finding invalid

Note that a frontrunner can increase the exchangeRate, but doing so doesn't seem to be favourable nor it was fully explained by the warden

[IllIllI000]

@GalloDaSballo lp.exchangeRate() -> lp.totalUnderlying() -> vault.getTotalUnderlying() -> vault._availableUnderlying() which is an abstract function. A vault may consider some portion of deposited funds as 'unavailable', and therefore the exchange rate may be manipulatable by deposit/withdrawal actions by an attacker

[GalloDaSballo]

@IllIllI000 with the information made available on the finding, I don't believe any specific exploit was detailed.

You can make the argument (post-fact) that _availableUnderlying can be improperly coded, potentially causing losses to a hypothetical vault, however that wouldn't be an issue with migration but a potential issue with how Vault Accounting is handled.

I don't believe you were making that argument in the original finding, so with the information I had at the time, given the fact that no POC showed how to manipulate the exchangeRate, I don't think the finding has proven that migrate is dangerous to end users