First Flight #4: Boss Bridge

Summary

With the current design, sendToL1 function is only called by withdrawTokensToL1. Moreover, withdrawTokensToL1 function doesn't do anything except calling sendToL1 function, with specific parameters. It seems clear that the purpose of this function is to be only called internally by withdrawTokensToL1 function, but its visibility is currently set to public.

Vulnerability Details

sendToL1 function is declared as public while it shouldn't. It should only be called by withdrawTokensToL1 function and therefore be internal or private.
In fact, this is important because withdrawTokensToL1 function is in charge of passing the message to be signed (before hashing) by the bridge operator.
Being able to call sendToL1 function only through withdrawTokensToL1 function is of paramount importance. Indeed, if it is possible to call sendToL1 directly, it could theoretically be possible to sign any message, as soon as the structure is respected (in order to successfully abi.decode the message):

Note that I added a _nonce parameter for signature replay attack.
So basically, any bridge operator could pass an address, a value (necessarily 0 as the contract is not able to receive ETH, except from selfdestruct), and some data. Finally, the following call will be executed successfully :

This means any signer (or bridge operator? I infer this is the same entity as the doc firstly talks about bridge operator, and then about signer in the "role" section) could abuse of the system, as he could do way more than simply sign messages containing data for a safeTransferFrom call.

Impact

The impact is HIGH as it allows any allowed signer to sign whatever message he wants, passing the data contained in the message to an external call. This is not what is expected from the bridge. Bridge operator should only be able to sign message containing a transferfrom instruction, in order to withdraw funds from L2 (i.e. transferfrom from L1Vault to user).

Moreover, NatSpec says about sendToL1 function:

This is non-sense as this bridge doesn't concern ETH. This suggests that :
(bool success,) = target.call{ value: value }(data); is meant to send back ether to user. This is non-sense as :

• this is not what this protocol is supposed to do

• this contract doesn't hold eth, doesn't have any payable function and doesn't implement any receive() or fallback() function, and thus will never be able to send ETH to anyone

Tools Used

Manual

Recommendations

I propose to modify the visibility of sendToL1 to make it internal while adding an underscore for clarity :

Another possibility would be to basically use only one function, such as (note that i added a nonce here to prevent signature replay attack) :

This doesn't lose clarity while doing all the work inside one fonction. The low level call is replaced by the direct use of transferFrom, or even safeTransferFrom, allowing to remove the check on the return value :

I also suggest to correct NatSpect to be consistent with what the code actually does.