Setting `Provider` address as `msg.sender` can result in permanant loss of funds
parth-15 opened this issue · 32 comments
Describe the bug
Any provider on L1 can fulfill cross trade request by calling provideCT function. So, the provider sends the _l1token to the user who have requested the tokens in L2 using requestRegisteredToken or requestNonRegisteredToken. To ensure that the provider gets the l2token of the requester, the L1CrossTrade contract constructs the message with claimCT signature so that the L1 provider can claim the requester's fund in L2.
crossTrade/contracts/L1/L1CrossTrade.sol
Lines 398 to 427 in fc11f6e
In the above function makeEncodeWithSignature, the argument to will be msg.sender(i.e. provider address). This can cause serious vulnerability when provider in L1 is not EOA but gnosis safe multisig(highly probable) ,contracts, smart accounts or smart wallets. In that case, claimCT will send funds to those addresses and it may be possible that those addresses can't be accessed in L2.
crossTrade/contracts/L2/L2CrossTrade.sol
Lines 213 to 218 in fc11f6e
The same issue can also be observed when the requester requests the tokens in L2 but the they don't have access to address through which they sent transaction in L2 on L1. This can be quite restrictive for the users. Also, this can prevent automated way of fulfilling requests through contract and bots and users would need to manually fulfill requests which can lead to less adoption.
To Reproduce
Steps to reproduce the behavior:
- User A deploys a gnosis safe multisig on L1 for becoming provider
- User B requests funds by calling
requestRegisteredTokenonL2 - User A fulfills the order and send the funds to the User B.
- User A calls
claimCTto get the fund on L2. The funds are sent to the address of gnosis safe multisig in L2. The address will be same as deployed on step 1. - User A can't access multisig wallets on L2 and funds are lost forever.
@parth-15 Yes, it is possible.
However, this is not an attack but rather a loss situation for Provide, and in a non-EOA situation, we tried to use Create2 to enable only contracts that use the same address. I think it would be better to discuss this matter and then make a decision.
Since it is not a situation where funds are being stolen, it would be good if you change the severity to MEDIUM.
@suahnkim What do you think about this matter?
@zzooppii I think this can be in category of permanant freezing of user funds. Funds aren't being stolen but can be frozen permanantly and can't be recovered which is high-impact issue.
Also, not all users are aware of Create2 but the common usecases will be multisig which can increase the impact of this issue.
Yes, thank you for your good comments.
@suahnkim What should I do if I do not receive funds due to the provider's mistake?
For example, if you create a contract and call the provide function, but the contract address is not in L2, you cannot receive funds.
I thought about this too, but at the time you had the onlyEOA modifier, so this wasn't a issue.
Can you tell me why onlyEOA modifier was originally there but was removed?
This is not loss of fund case, because the fact that the contract made a call knowing that they cannot claim on L2, it cannot be considered as a bug; since some contracts can receive and send on L2.
If this type of action is considered a bug, we would need to say standard bridge also has the same issue; but we don't say that. => severity is changed to LOW.
This is more of a feature request -> We just need to add another provide function that can specify the receiving L2 address.
For example, gnosis safe provides fund on L1, and specifies their L2 safe address when they provide.
function provideCTCustomRecipient
would be my suggestion
@usgeeus onlyEOA was removed to increase interoperability composability between different dapps (gnosis safe is one such example)
@suahnkim I disagree with proposed severity to low. I agree that this is not loss of funds for the L1 and L2 contract, but this can be loss or permanant freezing of funds to the users.
For EOAs (i.e., non-contract accounts), this is generally true that any account that can be accessed on Ethereum will also be
accessible on other EVM-based chains. However, this is not always true for contract-based accounts as the same
account/wallet address might be owned by different persons/entities on different chains. For instance, if a smart contract wallet factory deployed on both EVM-based chains uses deterministic CREATE2 which allows users to define its salt when deploying the wallet, Bob might use ABC as salt in L1 and Alice might use ABC as salt in L2. Both of them will end up getting the same wallet address on two different chains. A similar issue occurred in the Optimism-Wintermute Hack, but the actual incident is more complicated.
@parth-15 would you claim that optimism standard bridge + Arbitrum standard bridge for deposit or withdraw has the same vulnerability bug?
I don't think they would accept your report for it, as it is not something that can be fixed
I would say, by offering a function that allows to input custom address for receiving it resolves the issue of contracts not being able to use crossTrade to provide liquidity.
Which is why I would say this is more of a feature request to support custom recipient.
Contract creators should be aware of the issue if they are interacting cross chain -> not something that can be fixed from contract side.
And I would like to hear your proposal for fix.
Optimism standard bridge uses onlyEOA modifier on bridgeERC20 and bridgeETH, withdraw.
For your information, they don't have onlyEOA modifier on bridgeERC20To, bridgeETHto, withdrawTo to make contracts tradable.
So, a better solution is offer following 4 functions
- Request with onlyEOA modifier
- Provide with onlyEOA modifier
- Request custom recipient
- Provide custom recipient
@suahnkim I disagree with proposed severity to
low. I agree that this is not loss of funds for theL1andL2contract, but this can be loss or permanant freezing of funds to the users.For EOAs (i.e., non-contract accounts), this is generally true that any account that can be accessed on Ethereum will also be
accessible on other EVM-based chains. However, this is not always true for contract-based accounts as the same
account/wallet address might be owned by different persons/entities on different chains. For instance, if a smart contract wallet factory deployed on both EVM-based chains uses deterministic CREATE2 which allows users to define its salt when deploying the wallet, Bob might use ABC as salt inL1and Alice might use ABC as salt inL2. Both of them will end up getting the same wallet address on two different chains. A similar issue occurred in the Optimism-Wintermute Hack, but the actual incident is more complicated.
Could you also suggest a severity?
@suahnkim I feel this is High severity and not a feature request. Main reason for this is loss/freeze of funds for the smart wallets/multisig/contract accounts. The thing is we are not doing anything explicitly to prevent that in the contract despite knowing the issue. We generally can't rely on users to not use Cross Trade if they are contract accounts(multisig is common use case). If we want to prevent contract accounts to use this, then we should use onlyEOA modifier and we can stop that issue. The issue is High because we are allowing contract accounts knowing that they could lose the funds.
The issue would have been "feature request" if we had onlyEOA modifier and we are requesting feature to allow contract accounts.
I am thinking about fix and will add it next comment.
@suahnkim I feel this is
Highseverity and not a feature request. Main reason for this is loss/freeze of funds for the smart wallets/multisig/contract accounts. The thing is we are not doing anything explicitly to prevent that in the contract despite knowing the issue. We generally can't rely on users to not useCross Tradeif they are contract accounts(multisig is common use case). If we want to prevent contract accounts to use this, then we should useonlyEOAmodifier and we can stop that issue. The issue isHighbecause we are allowing contract accounts knowing that they could lose the funds.The issue would have been "feature request" if we had
onlyEOAmodifier and we are requesting feature to allowcontract accounts.I am thinking about fix and will add it next comment.
I am not sure if I agree with you reasoning, but that maybe because I am not familiar with classifying bug for L1<->L2 contracts. This issue seems to be very contract developer's ability dependent, and it may not be an issue if the contract developer does enough test to make sure the issue is not there. It is also possible that a contract cannot use this because their contract is not setup correctly. I think we can let @zzooppii decide in severity.
I think the fix is simple. We can add argument
_toin functionsrequestRegisteredToken,requestNonRegisteredTokenandprovideCT. We should also include this argument while generatinghashto ensure that users can't forge it.
I don't think we need to add that to the hash, because the message will be directly sent from L1 to L2 -> claimCT will need to support custom recipient OR create claimCTCustomRecipient don't need, because claimCT supports _from. And we need to record the custom recipient address on L1 storage; for possibly resending the message.
Hash does not need to include who the recipient is (we don't want the hash to change from creation of the request)
@suahnkim won't the provider be able to enter different value than customCT if we don't include it in hash or verify that it was indeed asked by the L2 requestor? L1 doesn't have any way to verify apart from adding it to hash.
@suahnkim won't the provider be able to enter different value than
customCTif we don't include it in hash or verify that it was indeed asked by theL2requestor?L1doesn't have any way to verify apart from adding it to hash.
try considering three cases:
- someone already provided CT: no one else can claim CT on L2 other than specified address.
- no one has provided CT: there will be no record of the provide CT based on the hash -> provide CT will go through (can be claimed only if the hash and the information to generate the hash is correct)
- requester edited their CT: there will be record of the hash (and all other relevant information for CT) on the storage -> provide CT will go through & cannot edit the CT amount & the provider provides the address to be received on L2 and L1->L2 message will be sent (cannot be modified)
@suahnkim @parth-15
There is no need to add it to the hash, just input who you want to give it to in L2 when provideCT.
And since our initial intention was to only use the same address, this can be seen as a suggestion for additional features rather than an error.
So, the severity will be set to LOW.
no need to add it to the hash, as it would modify the hash -> this would cause an issue on L2.
Custom address is guaranteed by only allowing L1->L2 message to originate from the L1 contract when provide is successful
the hash is used to only verify that the initial request is correct, any modification to the initial request is saved on L1 + any claim on L2 is dependent on action on L1.
@parth-15 if you don't understand my comment, please go through the code again. I don't think you understood the underlying security of the code.
@suahnkim Receiving L2 from an address other than the provider of L1 seems to be a simple matter, but receiving from L1 to an address other than the requester of L2 seems to have many things to consider.
What kind of work should I do?
no need to add it to the hash, as it would modify the hash -> this would cause an issue on L2. Custom address is guaranteed by only allowing L1->L2 message to originate from the L1 contract when provide is successful
the hash is used to only verify that the initial request is correct, any modification to the initial request is saved on L1 + any claim on L2 is dependent on action on L1.
@parth-15 if you don't understand my comment, please go through the code again. I don't think you understood the underlying security of the code.
@suahnkim I understood what you meant. It is crystal clear to me and I understand the security implications. What I was asking is alternate implementation of what @zzooppii mentioned here
no need to add it to the hash, as it would modify the hash -> this would cause an issue on L2. Custom address is guaranteed by only allowing L1->L2 message to originate from the L1 contract when provide is successful
the hash is used to only verify that the initial request is correct, any modification to the initial request is saved on L1 + any claim on L2 is dependent on action on L1.
@parth-15 if you don't understand my comment, please go through the code again. I don't think you understood the underlying security of the code.
what I was asking is modify L2 hash and not L1 hash. So, L2 hash consists of both msg.sender and _to address(where L2 requestor wants to receive tokens on L1. We would need to update provideCT arguments by adding two arguments( address1(where provider wants to receive an tokens on L2 and address2 where requestor receives funds on L1). In this way, we can match the hash and this shouldn't be an issue in my opinion. We would also need to incorporate other changes to make sure this works correctly.
What are your thoughts? @zzooppii
we want to keep L1 and L2 hash the same for the request, as it is the easiest way to keep track of the order (sales count is modifiable, so it is not a good way to keep track). Can you explain why it is an issue if we do not include it on the hash?
I think the conversation is getting too long, if you still don't understand it, please make a call.
p.s. When user requestCT for custom recipient (on L1), the receiver address storage will have to be not msg.sender, but the custom recipient address, basically only the custom recipient would have ability to edit or cancel the request on L1.
Here is an example snippet of my explanation => I ** ~ ** the part that requires edit
/// @notice Token transaction request
/// @param _l1token l1token Address
/// @param _l2token l2token Address
**/// @param _receiver when CT is provided, this defines the receiver Address. Only this address can modify or cancel the CT request after request is made**
/// @param _totalAmount Amount provided to L2
/// @param _ctAmount Amount to be received from L1
/// @param _saleCount Number generated upon request
/// @param _l1chainId chainId of l1token
function _request(
address _l1token,
address _l2token,
**address _receiver**
uint256 _totalAmount,
uint256 _ctAmount,
uint256 _saleCount,
uint256 _l1chainId
)
internal
{
if (_l2token == legacyERC20ETH) {
require(msg.value == _totalAmount, "CT: nativeTON need amount");
} else {
IERC20(_l2token).safeTransferFrom(msg.sender,address(this),_totalAmount);
}
bytes32 hashValue = getHash(
_l1token,
_l2token,
**_receiver**,
_totalAmount,
_ctAmount,
_saleCount,
_l1chainId
);
dealData[_saleCount] = RequestData({
l1token: _l1token,
l2token: _l2token,
**receiver: _receiver,**
provider: address(0),
totalAmount: _totalAmount,
ctAmount: _ctAmount,
chainId: _l1chainId,
hashValue: hashValue
});
emit CreateRequestCT(
_l1token,
_l2token,
**_receiver,**
_totalAmount,
_ctAmount,
_saleCount,
hashValue
);
}
}
I am sorry if my comment was not clear, but I thought it was very obvious.
So, lets check one by one for clarity:
- we generally do not want to modify the hash structure unless if we really need it. This is because we want the hash value to ensure that someone cannot modify the initial terms of the request.
Now, lets check on what we will have to do to support custom recipient for CT request:
When custom recipient version of request is called on L2
-> this indicates that the L1 fund control will be handled over to the specified custom address
- To edit the CT term -> we only allow the custom recipient (not the requester) to edit
- To cancel the CT-> we only allow the custom recipient (not the requester) to cancel. But, we probably should give option to whom the refund takes place; which means that cancel function on L1 must support the whom to refund to. Continuing from pervious conversations, this means that we need to support EOA only version of Cancel and custom recipient version of Cancel. And by extension, claimCT on L2 must support custom recipient as calldata.
I agree with all the things you mentioned. I was mainly worried by this indicates that the L1 fund control will be handled over to the specified custom address but I guess it's not an concern.
@parth-15
With regarding your concern, do you think the control should not be handed over to the custom recipient?
I don't think so because it will be the trusted entity by requestor. Was just trying to think in that direction but I guess that's not a concern.