V3.1 audit fixes #255
Merged
V3.1 audit fixes #255
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Coverage SummaryTotals
FilesExpand
|
|
Contract comparison - from 7eb68ea to fdcecd4
|
bridge-proxy: refund refactor
alyn509
previously approved these changes
Dec 2, 2024
andreiblt1304
previously approved these changes
Dec 2, 2024
CostinCarabas
dismissed stale reviews from andreiblt1304 and alyn509
The base branch was changed.
…fter-refund bridge-proxy: Clear storages after refund
bridge-proxy/src/bridge-proxy.rs
Outdated
|
|
||
| if call_data.endpoint.is_empty() | ||
| let non_empty_args = call_data.args.is_some(); | ||
| if (call_data.endpoint.is_empty() && non_empty_args) |
There was a problem hiding this comment.
Wrote an integration test for this. Apparently, if I send this RAW SC call data 000000000000000002faf08000 (empty function and 50 mil gas limit) it does not refund the transfer. Good catch @dragos-rebegea
| eth_tx.tx_nonce, | ||
| ); | ||
| // emit events only for non-SC destinations | ||
| if self.blockchain().is_smart_contract(ð_tx.to) { |
There was a problem hiding this comment.
To solve what @dragos-rebegea mentioned and fix the audit finding #9, do you think it would be better to do the following:
- emit the event only if the destination address is not a SC
- remove the
transfer_performed_sc_eventand instead emit thetransfer_performed_eventin the bridge proxy if the call to the SC succeeded
| multi_transfer_esdt_proxy, | ||
| }; | ||
|
|
||
| const MAX_BOARD_MEMBERS: usize = 40; |
There was a problem hiding this comment.
why 40? I would go to a higher value like 100 or something
There was a problem hiding this comment.
This is a fix for issue 4 from the Audit document.
We iterate over all board signatures and the call may run out of gas.
Maybe an integration test from your side could help us adjust this value.
multisig/src/setup.rs
Outdated
|
|
||
| #[only_owner] | ||
| #[payable("*")] | ||
| #[endpoint(bridgedTokensWrapperDepositLiquidity)] |
There was a problem hiding this comment.
why this? The owner of the wrapper is not the multisig contract
multisig/src/lib.rs
Outdated
|
|
||
| let action_ids_mapper = self.batch_id_to_action_id_mapping(eth_batch_id); | ||
|
|
||
| for act_id in action_ids_mapper.values() { |
There was a problem hiding this comment.
we should take max values here also
| transaction_details.refund_info.initial_nonce, | ||
| ); | ||
| } | ||
| let transaction_details = self.create_transaction_common(to, OptionalValue::None); |
There was a problem hiding this comment.
require call from bridge proxy
There was a problem hiding this comment.
Added + unit test.
multi-transfer-esdt/src/lib.rs
Outdated
| refund_tx_list.push(refund_tx); | ||
| self.token_with_transfer_role(eth_tx.token_id); | ||
| // let refund_tx = self.convert_to_refund_tx(eth_tx.clone()); | ||
| // refund_tx_list.push(refund_tx); |
bridged-tokens-wrapper/src/lib.rs
Outdated
| @@ -135,10 +134,18 @@ pub trait BridgedTokensWrapper: | |||
| self.token_decimals_num(&chain_specific_token_id).clear(); | |||
| } | |||
|
|
|||
| #[only_owner] | |||
There was a problem hiding this comment.
this is still present
dragos-rebegea
approved these changes
Dec 19, 2024
evelinemolnar
approved these changes
Dec 19, 2024
iulianpascalau
approved these changes
Dec 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference PR: multiversx/mx-bridge-eth-go#389