Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V3.1 audit fixes #255

Merged
merged 52 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
21d74ab
storage naming snake case -> camel case
CostinCarabas Nov 13, 2024
de92e66
multisig: add executed_actions storage
CostinCarabas Nov 13, 2024
81cdbfb
multisig: change_quorum improvement
CostinCarabas Nov 13, 2024
d9d15f1
wrapper: remove dfp_biguint tests
CostinCarabas Nov 14, 2024
bd18c31
wrapper: more audit fixes
CostinCarabas Nov 14, 2024
70a9a35
esdt-safe fix: distribute_fees
CostinCarabas Nov 19, 2024
50657a4
esdt-safe: Add failedRefund storage mapper
CostinCarabas Nov 19, 2024
939ad9d
code refactor, compilation, wasm files, Cargo.locks
CostinCarabas Nov 19, 2024
f25b8ef
esdt-safe: more audit fixes
CostinCarabas Nov 19, 2024
fe6ea58
bridge-proxy: audit fixes
CostinCarabas Nov 19, 2024
e8ad785
esdt-safe: fix unit tests
CostinCarabas Nov 19, 2024
46c4ab2
multi-transfer: unit tests fixes
CostinCarabas Nov 20, 2024
2a63507
multisig: code fixes + unit test fixes
CostinCarabas Nov 21, 2024
82684b2
bridge-proxy: allow empty endpoint only if empty args
CostinCarabas Nov 21, 2024
c7f9490
clippy, compilation, proxy fixes
CostinCarabas Nov 25, 2024
74b68f2
bridge-proxy: sc,test fixes
CostinCarabas Nov 25, 2024
0da9aa3
clean commented code
CostinCarabas Nov 25, 2024
a4f51fa
multi-transfer: batchTransferEsdtToken: refund if TransferRole
CostinCarabas Nov 27, 2024
dab4a14
multi-transfer: move_refund_batch_to_safe fix
CostinCarabas Nov 27, 2024
6c13007
multisig: clearActionsForBatchId
CostinCarabas Dec 2, 2024
2f1b424
esdt-safe: create_transaction add min_bridge_amount
CostinCarabas Dec 2, 2024
87441db
Merge pull request #256 from multiversx/bridge-proxy-refund-refactor
CostinCarabas Dec 2, 2024
0c4a1b2
Merge branch 'feat/v3.1' into v3.5-audit-fixes
CostinCarabas Dec 3, 2024
b620abe
Merge remote-tracking branch 'origin/feat/v3.1' into v3.5-audit-fixes
CostinCarabas Dec 3, 2024
bba190b
Merge remote-tracking branch 'origin/v3.5-audit-fixes' into v3.5-audi…
CostinCarabas Dec 3, 2024
05616f4
multisig: add bridgedTokensWrapperDepositLiquidity
CostinCarabas Dec 4, 2024
b560b2e
fixes after review
CostinCarabas Dec 5, 2024
52f720a
Fix tests
CostinCarabas Dec 6, 2024
bfc081c
test fixes
CostinCarabas Dec 6, 2024
d8637b2
bridge-proxy: Refactor refund storage and endpoint
CostinCarabas Dec 6, 2024
2924e4f
bridge-proxy: tests fixes
CostinCarabas Dec 6, 2024
7499d01
multi-transfer: test fixes
CostinCarabas Dec 6, 2024
65aba3d
multi-transfer: remove commented code
CostinCarabas Dec 6, 2024
566a2a0
bridge-proxy: refund unit test
CostinCarabas Dec 6, 2024
7cc699c
clippy fixes
CostinCarabas Dec 9, 2024
f18a048
multi-transfer: add unit test
CostinCarabas Dec 9, 2024
13d34c7
bridge-proxy: Clear storages after refund
CostinCarabas Dec 9, 2024
5f05e2e
bridge-proxy: Remove cooldown from refund
CostinCarabas Dec 9, 2024
13f8018
Merge pull request #263 from multiversx/bridge-proxy-clear-storages-a…
CostinCarabas Dec 10, 2024
8f1211f
multisig: clear_action fix
CostinCarabas Dec 10, 2024
3e1d993
Merge branch 'feat/v3.1' into v3.5-audit-fixes
CostinCarabas Dec 10, 2024
e6d3095
multisig: test fixes
CostinCarabas Dec 10, 2024
87f80bd
bridge-proxy: fix tests & lower gas limit for callback
CostinCarabas Dec 10, 2024
ad82d8f
Fixes after review
CostinCarabas Dec 12, 2024
227e6da
bridged-tokens-wrapper: fix unit test
CostinCarabas Dec 12, 2024
4f6a6eb
bridge-proxy: add unit test for empty endpoint and non-empty gas limit
CostinCarabas Dec 12, 2024
05515ec
Add events for finish execute
CostinCarabas Dec 12, 2024
7db4577
unit tests fixes after reviews applied
CostinCarabas Dec 12, 2024
247fb5b
multi-transfer: refactor batch_transfer
CostinCarabas Dec 12, 2024
38538b9
multisig: Fix unit tests
CostinCarabas Dec 12, 2024
ddb2146
Merge branch 'feat/v3.1' into v3.5-audit-fixes
CostinCarabas Dec 19, 2024
fdcecd4
Fixes after review
CostinCarabas Dec 19, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions bridge-proxy/src/bridge-proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ pub trait BridgeProxyContract:
} else {
CallData::default()
};

if call_data.endpoint.is_empty()
let non_empty_args = call_data.args.is_some();
if (call_data.endpoint.is_empty() && non_empty_args)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if i send empty endpoint and some arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the if statement would be executed and we would

            self.finish_execute_gracefully(tx_id);
            return;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@CostinCarabas CostinCarabas Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the condition to

if call_data.endpoint.is_empty()
            self.finish_execute_gracefully(tx_id);
            return;

Added unit test.

|| call_data.gas_limit < MIN_GAS_LIMIT_FOR_SC_CALL
|| call_data.gas_limit > MAX_GAS_LIMIT_FOR_SC_CALL
{
Expand Down Expand Up @@ -118,24 +118,27 @@ pub trait BridgeProxyContract:
self.tx().to(tx.to).payment(payment).transfer();
self.cleanup_transaction(tx_id);
}

#[promises_callback]
fn execution_callback(&self, #[call_result] result: ManagedAsyncCallResult<()>, tx_id: usize) {
if result.is_err() {
self.refund_transaction(tx_id);
let tx = self.get_pending_transaction_by_id(tx_id);
self.refund_transactions(tx_id).set(&tx);
dragos-rebegea marked this conversation as resolved.
Show resolved Hide resolved
}
self.cleanup_transaction(tx_id);
}

#[endpoint(refundTransaction)]
fn refund_transaction(&self, tx_id: usize) {
let tx = self.get_pending_transaction_by_id(tx_id);
let tx = self.refund_transactions(tx_id).get();
let esdt_safe_contract_address = self.get_esdt_safe_address();

let unwrapped_token = self.unwrap_token(&tx.token_id, tx_id);
let batch_id = self.batch_id(tx_id).get();
self.tx()
.to(esdt_safe_contract_address)
.typed(esdt_safe_proxy::EsdtSafeProxy)
.create_transaction(
.create_refund_transaction(
tx.from,
OptionalValue::Some(esdt_safe_proxy::RefundInfo {
address: tx.to,
Expand Down Expand Up @@ -184,7 +187,8 @@ pub trait BridgeProxyContract:
}

fn finish_execute_gracefully(&self, tx_id: usize) {
self.refund_transaction(tx_id);
let tx = self.get_pending_transaction_by_id(tx_id);
self.refund_transactions(tx_id).set(&tx);
self.cleanup_transaction(tx_id);
}

Expand Down
10 changes: 7 additions & 3 deletions bridge-proxy/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,21 @@ use transaction::EthTransaction;

#[multiversx_sc::module]
pub trait ConfigModule {
#[storage_mapper("pending_transactions")]
#[storage_mapper("pendingTransactions")]
fn pending_transactions(&self) -> MapMapper<usize, EthTransaction<Self::Api>>;

#[view(refundTransactions)]
#[storage_mapper("refundTransactions")]
fn refund_transactions(&self, tx_id: usize) -> SingleValueMapper<EthTransaction<Self::Api>>;

#[storage_mapper("payments")]
fn payments(&self, tx_id: usize) -> SingleValueMapper<EsdtTokenPayment<Self::Api>>;

#[storage_mapper("batch_id")]
#[storage_mapper("batchId")]
fn batch_id(&self, tx_id: usize) -> SingleValueMapper<u64>;

#[view(highestTxId)]
#[storage_mapper("highest_tx_id")]
#[storage_mapper("highestTxId")]
fn highest_tx_id(&self) -> SingleValueMapper<usize>;

#[storage_mapper("ongoingExecution")]
Expand Down
79 changes: 76 additions & 3 deletions bridge-proxy/tests/bridge_proxy_blackbox_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,77 @@ fn bridge_proxy_too_small_gas_sc_call_test() {
call_data: ManagedOption::some(call_data),
};

let amount = BigUint::from(500u64);
// Destination is not an initialized contract
evelinemolnar marked this conversation as resolved.
Show resolved Hide resolved
test.world
.tx()
.from(MULTI_TRANSFER_ADDRESS)
.to(BRIDGE_PROXY_ADDRESS)
.typed(bridge_proxy_contract_proxy::BridgeProxyContractProxy)
.deposit(&eth_tx, 1u64)
.egld_or_single_esdt(
&EgldOrEsdtTokenIdentifier::esdt(BRIDGE_TOKEN_ID),
0,
&amount,
)
.run();

test.world
.query()
.to(BRIDGE_PROXY_ADDRESS)
.typed(bridge_proxy_contract_proxy::BridgeProxyContractProxy)
.get_pending_transaction_by_id(1u32)
.returns(ExpectValue(eth_tx.clone()))
.run();

test.world
.tx()
.from(OWNER_ADDRESS)
.to(BRIDGE_PROXY_ADDRESS)
.typed(bridge_proxy_contract_proxy::BridgeProxyContractProxy)
.execute(1u32)
.run();

test.world
.check_account(BRIDGE_PROXY_ADDRESS)
.check_storage("str:refundTransactions|u32:1", "0x30313032303330343035303630373038303931300000000000000000050063726f7766756e64696e675f5f5f5f5f5f5f5f5f5f5f0000000d4252494447452d3132333435360000000201f4000000000000000101000000150000000466756e6400000000000f42400100000000")
.check_storage("str:batchId|u32:1", "1")
.check_storage("str:highestTxId", "1")
.check_storage("str:payments|u32:1", "nested:str:BRIDGE-123456|u64:0|biguint:500");
}

#[test]
fn bridge_proxy_empty_endpoint_with_args_test() {
let mut test = BridgeProxyTestState::new();

test.world.start_trace();

test.multisig_deploy();
test.deploy_bridge_proxy();
test.deploy_crowdfunding();
test.config_bridge();

let mut args = ManagedVec::new();
let call_data: CallData<StaticApi> = CallData {
endpoint: ManagedBuffer::new(),
gas_limit: GAS_LIMIT,
args: ManagedOption::some(args),
};

let call_data: ManagedBuffer<StaticApi> =
ManagedSerializer::new().top_encode_to_managed_buffer(&call_data);

let eth_tx = EthTransaction {
from: EthAddress {
raw_addr: ManagedByteArray::new_from_bytes(b"01020304050607080910"),
},
to: ManagedAddress::from(CROWDFUNDING_ADDRESS.eval_to_array()),
token_id: BRIDGE_TOKEN_ID.into(),
amount: BigUint::from(500u64),
tx_nonce: 1u64,
call_data: ManagedOption::some(call_data),
};

let amount = BigUint::from(500u64);
// Destination is not an initialized contract
evelinemolnar marked this conversation as resolved.
Show resolved Hide resolved
test.world
Expand Down Expand Up @@ -771,8 +842,10 @@ fn bridge_proxy_too_small_gas_sc_call_test() {
.execute(1u32)
.run();

// Refund: Funds are transfered to EsdtSafe
test.world
.check_account(ESDT_SAFE_ADDRESS)
.esdt_balance(BRIDGE_TOKEN_ID, amount.clone());
.check_account(BRIDGE_PROXY_ADDRESS)
.check_storage("str:refundTransactions|u32:1", "0x30313032303330343035303630373038303931300000000000000000050063726f7766756e64696e675f5f5f5f5f5f5f5f5f5f5f0000000d4252494447452d3132333435360000000201f4000000000000000101000000110000000000000000009896800100000000")
.check_storage("str:batchId|u32:1", "1")
.check_storage("str:highestTxId", "1")
.check_storage("str:payments|u32:1", "nested:str:BRIDGE-123456|u64:0|biguint:500");
}
12 changes: 12 additions & 0 deletions bridge-proxy/wasm/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions bridge-proxy/wasm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@

// Init: 1
// Upgrade: 1
// Endpoints: 9
// Endpoints: 10
// Async Callback (empty): 1
// Promise callbacks: 1
// Total number of exported functions: 13
// Total number of exported functions: 14

#![no_std]

Expand All @@ -23,8 +23,10 @@ multiversx_sc_wasm_adapter::endpoints! {
upgrade => upgrade
deposit => deposit
execute => execute
refundTransaction => refund_transaction
getPendingTransactionById => get_pending_transaction_by_id
getPendingTransactions => get_pending_transactions
refundTransactions => refund_transactions
highestTxId => highest_tx_id
pause => pause_endpoint
unpause => unpause_endpoint
Expand Down
4 changes: 2 additions & 2 deletions bridged-tokens-wrapper/scenarios/add_wrapped_token.scen.json
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,12 @@
"str:universalBridgedTokenIds.len": "1",
"str:universalBridgedTokenIds.index|nested:str:WUSDC-abcdef": "1",
"str:universalBridgedTokenIds.item|u32:1": "str:WUSDC-abcdef",
"str:token_decimals_num|nested:str:WUSDC-abcdef": "6"
"str:tokenDecimalsNum|nested:str:WUSDC-abcdef": "6"
},
"code": "file:../output/bridged-tokens-wrapper.wasm",
"owner": "address:owner"
}
}
}
]
}
}
97 changes: 2 additions & 95 deletions bridged-tokens-wrapper/scenarios/blacklist_token.scen.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,105 +19,12 @@
"gasLimit": "5,000,000",
"gasPrice": "0"
},
"expect": {
"status": "0",
"message": "",
"gas": "*",
"refund": "*"
}
},
{
"step": "scCall",
"txId": "unwrap-token",
"tx": {
"from": "address:user",
"to": "sc:bridged_tokens_wrapper",
"value": "0",
"esdt": {
"tokenIdentifier": "str:WUSDC-abcdef",
"value": "100"
},
"function": "unwrapToken",
"arguments": [
"str:USDC-aaaaaa"
],
"gasLimit": "5,000,000",
"gasPrice": "0"
},
"expect": {
"status": "4",
"message": "str:Esdt token unavailable",
"message": "str:Cannot blacklist token due to remaining liquidity",
"gas": "*",
"refund": "*"
}
},
{
"step": "checkState",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be kept as it is, onyl the unwrap-token and the expect message is different, right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Added the check

"accounts": {
"address:owner": {
"nonce": "4",
"balance": "0",
"storage": {}
},
"address:user": {
"nonce": "5",
"esdt": {
"str:USDC-aaaaaa": {
"balance": "200000000000000"
},
"str:USDC-bbbbbb": {
"balance": "500000000000000"
},
"str:USDC-cccccc": {
"balance": "400000000000000"
},
"str:WUSDC-abcdef": {
"balance": "900"
}
},
"storage": {}
},
"sc:bridged_tokens_wrapper": {
"nonce": "0",
"esdt": {
"str:WUSDC-abcdef": {
"balance": "1",
"roles": [
"ESDTRoleLocalMint",
"ESDTRoleLocalBurn"
]
},
"str:WUSDC-uvwxyz": {
"balance": "1",
"roles": [
"ESDTRoleLocalMint",
"ESDTRoleLocalBurn"
]
},
"str:USDC-aaaaaa": {
"balance": "300000000000000"
},
"str:USDC-cccccc": {
"balance": "100000000000000"
}
},
"storage": {
"str:chainSpecificTokenIds|nested:str:WUSDC-abcdef|str:.len": "1",
"str:chainSpecificTokenIds|nested:str:WUSDC-abcdef|str:.index|nested:str:USDC-cccccc": "1",
"str:chainSpecificTokenIds|nested:str:WUSDC-abcdef|str:.item|u32:1": "str:USDC-cccccc",
"str:chainSpecificToUniversalMapping|nested:str:USDC-cccccc": "str:WUSDC-abcdef",
"str:universalBridgedTokenIds.len": "1",
"str:universalBridgedTokenIds.index|nested:str:WUSDC-abcdef": "1",
"str:universalBridgedTokenIds.item|u32:1": "str:WUSDC-abcdef",
"str:tokenLiquidity|nested:str:USDC-aaaaaa": "300000000000000",
"str:tokenLiquidity|nested:str:USDC-cccccc": "100000000000000",
"str:token_decimals_num|nested:str:WUSDC-abcdef": "6",
"str:token_decimals_num|nested:str:USDC-cccccc": "18"
},
"code": "file:../output/bridged-tokens-wrapper.wasm",
"owner": "address:owner"
}
}
}
]
}
}
Loading