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. If moving funds, it is assumed someone in the multisig would catch an invalid or malicious address.
joestakey
0xean
Lines of code
https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L166-L174
Vulnerability details
Impact
https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L166-L174
As written in the solidity documentation, the low-level function call returns true as its first return value if the address called is non-existent, as part of the design of the EVM. Address existence must be checked prior to calling if needed. Since the function moveWithheldETH() is only called by "onlyByOwnGov", it won't be hard to check the address existence before doing the call.
Solidity documentation - https://docs.soliditylang.org/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions
Proof of Concept
The low-level function call is used in some places in the code and it can be problematic. For example, in the function moveWithheldETH() in the contract frxETHMinter there is a low level call, which gives the withheld ETH to the "to" address, but if the given address "to" doesn’t exist success will be equal to true and the function will return true and the code execution will continue like the call was successful.
Tools Used
VS Code
Recommended Mitigation Steps
Check before any low-level call, that the address actually exist. Since the function moveWithheldETH() is called only by the "onlyByOwnGov", it won't be hard to check that the address exists before the low-level call by checking its code size.