search

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

2 stars 1 forks source link

Admin rug vector in `moveWithheldETH()` #343

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/frxETHMinter.sol#L166-L170

Vulnerability details

According to the documentation, currentWithheldETH is meant to:

withhold part of the ETH deposit for future use, such as to earn yield in other places to supplement the ETH 2.0 staking yield

The issue is that the owner can call moveWithheldETH() with an arbitrary to, meaning a malicious owner can transfer it to one of their own addresses, effectively rugging the depositors.

Note that there is a difference with recoverEther, which is to be called in case of an emergency.

Here, we are making the assumption of the protocol running properly without an issue: the withheld ETH in frxETHMinter is expected to be used to earn additional yield. And as described above, the owner has the power of unexpectedly rug users by transferring the withheld ETH to a random address.

Impact

Medium

Tools Used

Manual Analysis

Mitigation

Consider keeping a whitelist of trusted/valid yield bearers contracts to transfer the withheld ETH to. A timelock would also be helpful to give users the time to react based on what they think of a new contract added to the whitelist. moveWithheldETH() would only be able to transfer ETH to an address on this whitelist.

FortisFortuna commented 2 years ago

We are well aware of the permission structure. The owner will most likely be a large multisig. We mentioned the Frax Multisig in the scope too.

joestakey commented 2 years ago

Duplicate of #107