28833 - [SC - Insight] Missing slippage protection in functions deposi...
Submitted on Feb 28th 2024 at 12:35:02 UTC by @MrPotatoMagic for Boost | Puffer Finance
Report ID: #28833
Report type: Smart Contract
Report severity: Insight
Target: https://etherscan.io/address/0x7276925e42f9c4054afa2fad80fa79520c453d6a
Impacts:
Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)
Description
Brief/Intro
The functions depositWstETH()
and depositStETH()
in PufferDepositor.sol and the functions deposit()
and mint()
in PufferVault.sol allow users to obtain pufETH tokens. But these functions do not allow the user to pass in a minAmountOut
parameter to specify the minimum amount of shares they want to receive. Due to this, users will be prone to intentional MEV attacks and unintentional slippage from other users., which will cause them to receive less than expected shares.
Note: This issue is different from that reported in the Blocksec report, which talks about slippage in the swap functions only (which are OOS).
References
Blocksec report (see details of 2.3.1 for proof) - https://github.com/blocksecteam/audit-reports/blob/main/solidity/blocksec_puffer_v1.0-signed.pdf
Additionally, the ERC4626 EIP mentions that: If implementors intend to support EOA account access directly, they should consider adding an additional function call for deposit/mint/withdraw/redeem with the means to accommodate slippage loss or unexpected deposit/withdrawal limits, since they have no other means to revert the transaction if the exact output amount is not achieved.
EIP4626 - https://eips.ethereum.org/EIPS/eip-4626#security-considerations
Vulnerability Details
First, let's look at the functions
depositWstETH()
anddepositStETH()
. Both functions currently only take in WstETH or StETH and provide users with pufETH, without verifying if the return value of pufETH amount received is atleast of a certain minimum amount. This is due to a lack ofminAmountOut
parameter, which checks if thepufETHAmount
returned is greater than equal to it. If true, we should finish execution but if not then the execution should be reverted.
File: PufferDepositor.sol
158: /**
159: * @inheritdoc IPufferDepositor
160: */
161: function depositWstETH(Permit calldata permitData) external restricted returns (uint256 pufETHAmount) {
162: try ERC20Permit(address(_WST_ETH)).permit({
163: owner: msg.sender,
164: spender: address(this),
165: value: permitData.amount,
166: deadline: permitData.deadline,
167: v: permitData.v,
168: s: permitData.s,
169: r: permitData.r
170: }) { } catch { }
171:
172: SafeERC20.safeTransferFrom(IERC20(address(_WST_ETH)), msg.sender, address(this), permitData.amount);
173: uint256 stETHAmount = _WST_ETH.unwrap(permitData.amount);
174:
175: return PUFFER_VAULT.deposit(stETHAmount, msg.sender);
176: }
177:
178: /**
179: * @inheritdoc IPufferDepositor
180: */
181:
182: function depositStETH(Permit calldata permitData) external restricted returns (uint256 pufETHAmount) {
183: try ERC20Permit(address(_ST_ETH)).permit({
184: owner: msg.sender,
185: spender: address(this),
186: value: permitData.amount,
187: deadline: permitData.deadline,
188: v: permitData.v,
189: s: permitData.s,
190: r: permitData.r
191: }) { } catch { }
192:
193: SafeERC20.safeTransferFrom(IERC20(address(_ST_ETH)), msg.sender, address(this), permitData.amount);
194: return PUFFER_VAULT.deposit(permitData.amount, msg.sender);
195: }
Second. let's take a look at the function
deposit()
andmint()
in PufferVault.sol. These functions are publicly accessible and allow users (who have already approved the vault with StETH) to obtain pufETH tokens. The same issue mentioned above applies to these functions as well. In case of mint(), the call would revert since the slippage requires the user to approve more assets to the vault contract.
File: PufferVault.sol
085: /**
086: * @inheritdoc ERC4626Upgradeable
087: * @dev Restricted in this context is like `whenNotPaused` modifier from Pausable.sol
088: */
089: function deposit(
090: uint256 assets,
091: address receiver
092: ) public virtual override restricted returns (uint256) {
093: return super.deposit(assets, receiver);
094: }
095:
096: /**
097: * @inheritdoc ERC4626Upgradeable
098: * @dev Restricted in this context is like `whenNotPaused` modifier from Pausable.sol
099: */
100: function mint(
101: uint256 shares,
102: address receiver
103: ) public override restricted returns (uint256) {
104: return super.mint(shares, receiver);
105: }
Impact Details
The issue has been marked as Medium-severity since:
When a user uses functions
depositWstETH()
,depositStETH()
anddeposit
, they receive lesser shares than expected due to the slippage.When a user uses function
mint()
, the slippage could cause the user to receive lesser shares or entirely revert due to enough assets not being approved.This is mentioned as a security consideration in EIP4626.
Proof of Concept
How to use this POC:
Add the POC to PufferVaultMainnet.fork.t.sol
At the top of the file, add { Test , console} to the namespace. The same is needed to be done in the TestHelper.sol file.
Add the RPC url for mainnet in setUp() in TestHelper.sol
Run the POC using
forge test --mt testMissingSlippageIssue -vvv
File: PufferVaultMainnet.fork.t.sol
087: // Actors
088: address public jack = makeAddr("jack");
089: address public jill = makeAddr("jill");
090:
091: function testMissingSlippageIssue() public giveToken(BLAST_DEPOSIT, address(stETH), jack, 50 ether) giveToken(BLAST_DEPOSIT, address(stETH), jill, 50 ether) {
092:
093: vm.prank(jack);
094: uint256 jackExpectedShares = pufferVault.previewDeposit(50 ether);
095: console.log("Jack's expected shares:", jackExpectedShares);
096:
097: vm.prank(jill);
098: stETH.approve(address(pufferVault), type(uint256).max);
099: vm.prank(jill);
100: uint256 jillReceivedShares = pufferVault.depositStETH(50 ether, jill);
101: console.log("Jill's received shares:", jillReceivedShares);
102:
103: // Let's say the vault receives some tokens from claiming
104: vm.deal(address(pufferVault), 50 ether);
105:
106: vm.prank(jack);
107: stETH.approve(address(pufferVault), type(uint256).max);
108: vm.prank(jack);
109: uint256 jackReceivedShares = pufferVault.depositStETH(50 ether, jack);
110: console.log("Jack's received shares:", jackReceivedShares);
111:
112: console.log("Jack's slippage:", jackExpectedShares - jackReceivedShares);
113: }
[PASS] testMissingSlippageIssue() (gas: 391707)
Logs:
Jack's expected shares: 49905530654562557002
Jill's received shares: 49905530654562557002
Jack's received shares: 49898438881924232864
Jack's slippage: 7091772638324138
Last updated
Was this helpful?