Open code423n4 opened 1 year ago
trust1995 marked the issue as primary issue
trust1995 marked the issue as satisfactory
0xLightt marked the issue as sponsor acknowledged
trust1995 marked the issue as selected for report
The reason this is not being addressed directly in this contract is we prefer to use a periphery contract like a generalized ERC4626 router to account for slippage and deadlines.
Lines of code
https://github.com/code-423n4/2023-05-maia/blob/53c7fe9d5e55754960eafe936b6cb592773d614c/src/erc-4626/ERC4626DepositOnly.sol#L32-L44 https://github.com/code-423n4/2023-05-maia/blob/53c7fe9d5e55754960eafe936b6cb592773d614c/src/erc-4626/ERC4626DepositOnly.sol#L47-L58 https://github.com/code-423n4/2023-05-maia/blob/53c7fe9d5e55754960eafe936b6cb592773d614c/src/ulysses-amm/UlyssesRouter.sol#L49-L56
Vulnerability details
Impact
https://eips.ethereum.org/EIPS/eip-4626#security-considerations 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." Using thebHermes
contract that inherits theERC4626DepositOnly
contract, EOAs can call the followingERC4626DepositOnly.deposit
andERC4626DepositOnly.mint
functions directly. However, because no slippage controls can be specified when calling these functions, these functions'shares
andassets
outputs can be less than expected to these EOAs.https://github.com/code-423n4/2023-05-maia/blob/53c7fe9d5e55754960eafe936b6cb592773d614c/src/erc-4626/ERC4626DepositOnly.sol#L32-L44
https://github.com/code-423n4/2023-05-maia/blob/53c7fe9d5e55754960eafe936b6cb592773d614c/src/erc-4626/ERC4626DepositOnly.sol#L47-L58
In contrast, the following
UlyssesRouter.addLiquidity
function does control the slippage by including theminOutput
input and executingamount = ulysses.deposit(amount, msg.sender)
andif (amount < minOutput) revert OutputTooLow()
. Although such slippage control for an ERC-4626 vault exists in this protocol's other contract, it does not exist in thebHermes
contract. As a result, EOAs can mint less bHermes shares than expected when calling thebHermes
contract'sERC4626DepositOnly.deposit
function and send and burn more HERMES tokens than expected when calling thebHermes
contract'sERC4626DepositOnly.mint
function.https://github.com/code-423n4/2023-05-maia/blob/53c7fe9d5e55754960eafe936b6cb592773d614c/src/ulysses-amm/UlyssesRouter.sol#L49-L56
Proof of Concept
The following steps can occur for the described scenario involving the
bHermes
contract'sERC4626DepositOnly.mint
function. The case involving thebHermes
contract'sERC4626DepositOnly.deposit
function is similar to this.bHermes
contract'sERC4626DepositOnly.mint
function with theshares
input being 1e18.ERC4626DepositOnly.mint
function call causes 1.2e18 HERMES tokens to be transferred from Alice.Tools Used
VSCode
Recommended Mitigation Steps
The
bHermes
contract can be updated to include adeposit
function that allowsmsg.sender
to specify the minimum bHermes shares to be minted for calling the correspondingERC4626DepositOnly.deposit
function; calling suchbHermes.deposit
function should revert if the correspondingERC4626DepositOnly.deposit
function'sshares
output is less than the specified minimum bHermes shares to be minted. Similarly, thebHermes
contract can also include amint
function that allowsmsg.sender
to specify the maximum HERMES tokens to be sent for calling the correspondingERC4626DepositOnly.mint
function; calling suchbHermes.mint
function should revert if the correspondingERC4626DepositOnly.mint
function'sassets
output is more than the specified maximum HERMES tokens to be sent.Assessed type
Other