Open code423n4 opened 2 years ago
Although we recongize this could be a user generated issue we disagree with the severity and think it should be downgraded to QA. We do intend to add an explicit check for the lengths of the two arrays to be equal as recommended.
This is actually a duplicate of #113 which is marked as QA
Agree this is QA for input sanitization.
Lines of code
https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxy.sol#L127-L147
Vulnerability details
Impact
A call to
MIMOProxy.multicall
may terminate without performing all the actions expected by the user. This can leave the vault in a state where it leaks value if it performing an action which opens a temporary MEV opportunity.Proof of Concept
If we snip away access control and error handling from
MIMOProxy.multicall
we have the function as shown:Note that we write
targets.length
elements into an array of lengthdata.length
.Consider the case where a user wants to perform an action temporarily creates a MEV opportunity. e.g. interacting with a contract which performs "lazy" minting of tokens such as Element finance.
In this example the contract would perform two calls, one to the token contract to perform a transfer to Element finance and another to mint the corresponding amount of wrapped position tokens, and the arguments (in psuedocode) are then
However if the user manages to mangle their arguments so that they actually pass
then
multicall
will still accept this input but will terminate after the first external call, the remaining elements of thedata
array will not be used.MIMOProxy
will then transfer the tokens out of the user's vault but not receive anything in return.Recommended Mitigation Steps
Add an explicit check for the lengths of these two arrays to be equal.
Funnily enough, the custom error
CustomErrors.TARGETS_LENGTH_DIFFERENT_THAN_DATA_LENGTH
already exists but is unused.