search

code-423n4 / 2022-09-frax-findings

2 stars 1 forks source link

Users cannot use `mintWithSignature()` function in most cases #365

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L84

Vulnerability details

Impact

Function sfrxETH.mintWithSignature() allows users to approve and mint in one transaction. Users will provide input param shares and function will calculate what assets amount needed to be approved (in case approveMax = false).

uint256 amount = approveMax ? type(uint256).max : previewMint(shares);
asset.permit(msg.sender, address(this), amount, deadline, v, r, s);
return (mint(shares, receiver));

It used previewMint() of ERC4626 to calculate amount from shares input param. Users need to sign a message with this amount value off-chain.

But since in design, the exchange rate of sfrxETH will increase linearly over time (every second), there is no way users can calculate the exact value of amount from share off-chain. For example, users calculate amount = X given shares = Y off-chain and sign amount = X. But when transaction is executed, the exchange rate is changed, amount > X given shares = Y.

The result is user’s signature invalid and cannot be executed.

Proof of Concept

Consider the scenario

  1. Alice wants to approve and mint 100 sfrxETH in 1 transaction. She got the exchange rate from the contract 1 frxETH = 1 sfrxETH. So she signs amount = 100 to approve 100 frxETH. Note that Alice doesn’t want to approve max since it’s more risky and not recommended from her expert crypto friends.

  2. She sends the transaction and waits for it to be executed. But by the time it’s executed, the exchange rate is updated 1.1 frxETH = 1 sfrxETH, which means if she wants to mint 100 sfrxETH she needs to approve 110 frxETH. So her transaction is reverted.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider add 1 more input param amount in mintWithSignature() function. Or only allow users to approve max.

FortisFortuna commented 2 years ago

Technically correct, though in practice, we will allow user-defined slippage on the UI

FortisFortuna commented 2 years ago

duplicate of https://github.com/code-423n4/2022-09-frax-findings/issues/100

0xean commented 2 years ago

closing as dupe of #35