test(evm): additional tests and refactors #2176
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several enhancements to the Nibiru EVM project, focusing on testing and infrastructure improvements. The changes include adding new smart contracts for testing dirty state attacks, updating test utilities, modifying bank and inflation-related functionalities, and refining various test suites. The modifications aim to improve the robustness of the EVM module, particularly in handling contract interactions and state management. Changes
Sequence DiagramsequenceDiagram
participant EVM as EVM Contract
participant Wasm as WASM Precompile
participant Bank as Bank Module
EVM->>Wasm: attack(wasmAddr, msgArgs)
Wasm->>Bank: Execute bank transfer
Bank-->>Wasm: Transfer confirmation
Wasm-->>EVM: Operation complete
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| stakingGenesis := stakingtypes.NewGenesisState(stakingtypes.DefaultParams(), validators, delegations) | ||
| stakingParams := stakingtypes.DefaultParams() | ||
| stakingParams.BondDenom = appconst.BondDenom | ||
| stakingGenesis := stakingtypes.NewGenesisState(stakingParams, validators, delegations) |
There was a problem hiding this comment.
I needed the stake denom to be unibi instead of stake for the wasm precompile test that delegates.
| @@ -89,6 +89,38 @@ func (bk NibiruBankKeeper) UndelegateCoins( | |||
| ) | |||
| } | |||
|
|
|||
| func (bk NibiruBankKeeper) DelegateCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error { | |||
There was a problem hiding this comment.
We were missing these overrides from the bank BaseKeeper.
| @@ -22,12 +22,6 @@ func (s *StateDB) DebugDirties() map[common.Address]int { | |||
| return s.Journal.dirties | |||
| } | |||
|
|
|||
| // DebugEntries is a test helper that returns the sequence of [JournalChange] | |||
| // objects added during execution. | |||
| func (s *StateDB) DebugEntries() []JournalChange { | |||
| s.EqualValues(deps.Sender.NibiruAddr.String(), typedResp.Owner) | ||
| } | ||
|
|
||
| func AssertWasmCounterStateWithEvm( |
There was a problem hiding this comment.
One way of asserting the counter via wasmKeeper directly, and one way of asserting the counter via EVM wasm precompile call.
| @@ -34,11 +38,48 @@ func TestWasmSuite(t *testing.T) { | |||
| suite.Run(t, new(WasmSuite)) | |||
| } | |||
|
|
|||
| func (s *WasmSuite) TestExecuteHappy() { | |||
| func (s *WasmSuite) TestInstantiate() { | |||
There was a problem hiding this comment.
I added this because I tore out the instantiating via wasm precompile in the SetupWasmContracts() test method.
In general, it's bad practice to rely on test setup code to validate behavior of production code.
| // bytes memory key | ||
| // ) external view returns (bytes memory response); | ||
| // ``` | ||
| func (s *WasmSuite) assertWasmCounterStateRaw( |
| @@ -500,11 +556,11 @@ func (s *WasmSuite) TestExecuteMultiPartialExecution() { | |||
| deps := evmtest.NewTestDeps() | |||
| evmObj, _ := deps.NewEVM() | |||
|
|
|||
| wasmContracts := test.SetupWasmContracts(&deps, evmObj, &s.Suite) | |||
| wasmContracts := test.SetupWasmContracts(&deps, &s.Suite) | |||
There was a problem hiding this comment.
One benefit is that we don't need to pass in evmObj to deploy and instantiate wasm contracts anymore. That's useful in test suites where we only want wasm contracts and not EVM.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
x/evm/precompile/funtoken_test.go (3)
154-172: Consider adding edge cases to the ConvertCoinToEvm test.While the happy path is well tested, consider adding test cases for:
- Converting zero amount
- Converting more than available balance
- Converting with invalid addresses
Line range hint
377-400: Enhance error messages in test assertions.The test verifies the sendToEvm functionality well, but the error messages could be more descriptive. Consider adding more context about the expected state transitions.
- s.Require().Empty(ethTxResp.VmError, "sendToEvm VMError") + s.Require().Empty(ethTxResp.VmError, "sendToEvm should not return VM error when converting valid amount")
530-551: Add documentation for test scenarios.While the test implementation is good, consider adding comments explaining:
- Initial token distribution
- Expected state after each operation
- Purpose of using specific token amounts
x/evm/precompile/test/export.go (3)
48-55: Parameterize contract instantiations if needed.
Currently, the instantiation messages are empty ({}). If future tests need customization, consider using more specific instantiate messages.
59-59: Logging is helpful.
The log statement aids debugging. Ensure consistency in logging verbosity across the codebase.
125-125: Explicit permission keeper usage.
Creation of awasmPermissionedKeeperis straightforward. Just confirm test vs. production usage to avoid opening undesired access in real deployments.x/evm/embeds/contracts/TestDirtyStateAttack4.sol (2)
8-9: Remove unused imports.The contract doesn't use the Strings or ERC20 imports.
-import "@openzeppelin/contracts/utils/Strings.sol"; -import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
16-23: Add event emission for counter increments and use unchecked for gas optimization.Track state changes with events and optimize gas usage for the counter increment.
+ event CounterIncremented(uint256 newValue); + function attack(string calldata wasmAddr, bytes calldata msgArgs) external { - counter++; + unchecked { + counter++; + } + emit CounterIncremented(counter);x/evm/embeds/contracts/TestDirtyStateAttack5.sol (2)
8-9: Remove unused imports.The contract doesn't use the Strings or ERC20 imports.
-import "@openzeppelin/contracts/utils/Strings.sol"; -import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
14-19: Add input validation and use constants for amounts.Validate input parameters and define constants for better maintainability.
+ uint256 private constant ATTACK_AMOUNT = 5e6; // 5 NIBI + function attack(string calldata wasmAddr, bytes calldata msgArgs) external { + require(bytes(wasmAddr).length > 0, "Empty WASM address"); INibiruEvm.BankCoin[] memory funds = new INibiruEvm.BankCoin[](1); - funds[0] = INibiruEvm.BankCoin({denom: "unibi", amount: 5e6}); // 5 NIBI + funds[0] = INibiruEvm.BankCoin({denom: "unibi", amount: ATTACK_AMOUNT});x/evm/embeds/contracts/TestDirtyStateAttack3.sol (1)
40-49: Add error details to revert message.The error message should include more details about the failure to aid in debugging.
- require(success, string.concat("Failed to call bankSend")); + require(success, string.concat("Failed to call bankSend for token: ", Strings.toHexString(erc20)));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
x/evm/precompile/test/bank_transfer.wasmis excluded by!**/*.wasmx/evm/precompile/test/counter.wasmis excluded by!**/*.wasmx/evm/precompile/test/hello_world_counter.wasmis excluded by!**/*.wasmx/evm/precompile/test/infinite_loop.wasmis excluded by!**/*.wasmx/evm/precompile/test/staking.wasmis excluded by!**/*.wasmx/evm/precompile/test/wasteful_gas.wasmis excluded by!**/*.wasm
📒 Files selected for processing (21)
CHANGELOG.md(1 hunks)x/common/testutil/testapp/test_util.go(4 hunks)x/common/testutil/testapp/testapp.go(2 hunks)x/evm/embeds/artifacts/contracts/TestDirtyStateAttack1.sol/TestDirtyStateAttack1.json(1 hunks)x/evm/embeds/artifacts/contracts/TestDirtyStateAttack2.sol/TestDirtyStateAttack2.json(1 hunks)x/evm/embeds/artifacts/contracts/TestDirtyStateAttack3.sol/TestDirtyStateAttack3.json(1 hunks)x/evm/embeds/artifacts/contracts/TestDirtyStateAttack4.sol/TestDirtyStateAttack4.json(1 hunks)x/evm/embeds/artifacts/contracts/TestDirtyStateAttack5.sol/TestDirtyStateAttack5.json(1 hunks)x/evm/embeds/contracts/TestDirtyStateAttack1.sol(1 hunks)x/evm/embeds/contracts/TestDirtyStateAttack2.sol(1 hunks)x/evm/embeds/contracts/TestDirtyStateAttack3.sol(1 hunks)x/evm/embeds/contracts/TestDirtyStateAttack4.sol(1 hunks)x/evm/embeds/contracts/TestDirtyStateAttack5.sol(1 hunks)x/evm/embeds/embeds.go(3 hunks)x/evm/keeper/bank_extension.go(1 hunks)x/evm/keeper/funtoken_from_coin_test.go(2 hunks)x/evm/precompile/funtoken_test.go(11 hunks)x/evm/precompile/test/export.go(9 hunks)x/evm/precompile/wasm_test.go(8 hunks)x/evm/statedb/debug.go(0 hunks)x/evm/statedb/journal_test.go(4 hunks)
💤 Files with no reviewable changes (1)
- x/evm/statedb/debug.go
✅ Files skipped from review due to trivial changes (5)
- x/evm/embeds/artifacts/contracts/TestDirtyStateAttack3.sol/TestDirtyStateAttack3.json
- x/evm/embeds/artifacts/contracts/TestDirtyStateAttack4.sol/TestDirtyStateAttack4.json
- x/evm/embeds/artifacts/contracts/TestDirtyStateAttack5.sol/TestDirtyStateAttack5.json
- x/evm/embeds/artifacts/contracts/TestDirtyStateAttack2.sol/TestDirtyStateAttack2.json
- x/evm/embeds/artifacts/contracts/TestDirtyStateAttack1.sol/TestDirtyStateAttack1.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (45)
x/evm/precompile/funtoken_test.go (5)
122-122: LGTM! Good use of constants.Using
evm.EVMBankDenomconstant instead of string literals improves maintainability and reduces the risk of typos.Also applies to: 129-129
464-481: LGTM! Comprehensive balance checks.The test thoroughly verifies balances for both recipient and module accounts after the transfer.
555-584: LGTM! Well-structured token transfer test.The test properly verifies:
- Token escrow mechanism
- Balance updates for both sender and receiver
- Module account balance
608-620: LGTM! Thorough verification of final state.Good job verifying:
- Bank balances are zeroed
- ERC20 tokens are properly transferred
- Module account balances are correct
Line range hint
486-489: Verify consistent token amount handling across tests.Let's verify that all token amount calculations properly handle decimals.
✅ Verification successful
Token decimal handling is consistent across the codebase ✅
The bigTokens helper and NativeToWei functions are consistently used for token amount calculations throughout the tests and implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct big.Int operations that might not handle token decimals properly rg -n "big\.NewInt\([0-9]" --type go # Search for potential decimal handling in token transfers ast-grep --pattern 'big.NewInt($amount)'Length of output: 37632
x/evm/precompile/test/export.go (16)
31-31: Function signature looks clear.
The newSetupWasmContractsfunction signature is succinct and focuses on instantiating contracts. No issues observed in parameter usage.
61-69: Instantiation permissions and fund allocation.
Instantiation code looks conventional. However, if these contracts require an initial balance or additional parameters, consider passing them here.
84-84: Renaming parameter to 'app'.
Consistent naming (e.g.,app *app.NibiruApp) helps readability and aligns with common Go conventions.
109-111: Staking bytecode path.
No immediate issues. Ensure it’s the correct compiled artifact.
161-170: Smart query logic.
The query approach for verifying the counter state is correct and clean. The error and response checks look solid.
172-184: EVM-based assertion utility.
IntroducingAssertWasmCounterStateWithEvmis tidy and helps unify EVM context checks. Good separation from the direct Wasm queries.
192-192: Error handling is consistent.
TheNoError(err)usage follows standard test patterns.
202-202: Return data validation.
EnsuringevmResp.Retis not empty is a good defensive check.
205-209: Unpacking the ABI response
The approach is straightforward. No suggestions here.
280-280: Empty line review
No changes needed here.
282-289: Building executeMsgs slice
This loop is clear and maintainable for appending execution messages.
290-290: Input packing
Packaging input viaembeds.SmartContract_Wasm.ABI.Packis consistent with typical usage.
292-292: Use ofWasmMethod_executeMulti
This keeps multi-call logic simpler. Good approach.
302-302: Return data check after multi-call
Ensuring the return data is non-empty helps confirm successful execution.
103-104: Bytecode path check.
Paths look correct for importinghello_world_counter.wasm. Consider verifying the file presence in CI.✅ Verification successful
WASM file path verified successfully
The filehello_world_counter.wasmexists at the specified pathx/evm/precompile/test/hello_world_counter.wasm.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify "hello_world_counter.wasm" file presence in the repo. fd hello_world_counter.wasmLength of output: 76
105-108: Bank transfer bytecode path.
Likewise, verify the "bank_transfer.wasm" file is present if other tests or references rely upon it.✅ Verification successful
Bank transfer WASM file is present and correctly referenced ✅
The file exists at the exact path specified in the code:x/evm/precompile/test/bank_transfer.wasm🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify "bank_transfer.wasm" file presence in the repo. fd bank_transfer.wasmLength of output: 64
x/evm/statedb/journal_test.go (10)
172-172: Inserting Wasm setup before EVM creation
By setting up Wasm contracts prior to EVM initialization, you ensure the code and environment are consistent.
177-177: Re-initializing EVM
Creating a newevmObjandstateDBafter Wasm setup is logical.
187-187: Increment without commit
This call ensures pending changes remain in the journal. Good coverage for reversion tests.
197-201: Dual assertion approach
Verifying withAssertWasmCounterStateWithEvmvs.AssertWasmCounterStateclarifies EVM vs. Cosmos state. Nice coverage.
203-203: Checking uncommitted state in Cosmos
Confirms the state remains at the prior level. Good test coverage.
210-217: Multiple increments pre-commit
Demonstrates snapshot reversion from multiple pending changes. No concerns.
227-228: Revert to snapshot ID=2
Restores partial increments. Well-structured test for partial state revert.
232-234: Revert to snapshot ID=0
Validation of returning fully to the earliest state. Good demonstration.
237-237: Final commit
Ensures ending with a clean state. Straightforward.
240-240: Cosmos state check
Ensures that after commit, the cosmos state aligns with the final EVM context.x/common/testutil/testapp/test_util.go (1)
45-45: LGTM! Consistent use of bond denomination.The changes standardize the bond denomination usage across the codebase by using
appconst.BondDenominstead ofsdk.DefaultBondDenom.Also applies to: 97-99, 110-110, 116-116
x/common/testutil/testapp/testapp.go (1)
20-20: LGTM! Proper integration of crypto codec.The addition of crypto codec registration ensures proper handling of cryptographic interfaces.
Also applies to: 130-130
x/evm/keeper/bank_extension.go (1)
92-106: LGTM! Verify state synchronization in tests.The implementation of delegation methods is correct and follows the established pattern of using ForceGasInvariant. The state synchronization is properly handled for both sender and recipient addresses.
Let's verify the test coverage for these new methods:
Also applies to: 108-122
x/evm/embeds/embeds.go (3)
50-59: LGTM: New test contract JSON embeddings.The byte slice variables are properly defined with descriptive names and appropriate
go:embeddirectives.
167-191: LGTM: Well-documented test contract definitions.Each test contract is properly defined with clear documentation describing its purpose and behavior.
211-215: LGTM: Proper initialization of test contracts.All new contracts are correctly initialized using
MustLoad()in theinitfunction.x/evm/precompile/wasm_test.go (4)
41-76: LGTM: Well-structured test for contract instantiation.The test properly verifies contract instantiation with appropriate assertions and error handling.
173-211: LGTM: Comprehensive query tests.Both
TestQueryRawandTestQuerySmartthoroughly test the query functionality with proper response validation.
602-671: LGTM: Well-documented dirty state attack test.
TestWasmPrecompileDirtyStateAttack4thoroughly tests the bank transfer scenario with clear initial state, actions, and expected outcomes.
674-771: LGTM: Comprehensive staking attack test.
TestWasmPrecompileDirtyStateAttack5properly tests the staking scenario with appropriate validations of balances and delegations.x/evm/keeper/funtoken_from_coin_test.go (3)
774-870: LGTM: Well-structured test for combined transfer operations.
TestDirtyStateAttack1thoroughly tests the combination of manual send and FunToken.sendToBank with proper balance assertions.
872-959: LGTM: Comprehensive test for ERC20 operations.
TestDirtyStateAttack2properly tests the combination of ERC20 transfer and FunToken.sendToBank with appropriate balance checks.
961-1059: LGTM: Thorough test for revert scenarios.
TestDirtyStateAttack3effectively tests the revert behavior with proper state validation before and after the revert.CHANGELOG.md (1)
90-90: LGTM: Clear changelog entry.The changelog entry properly documents the addition of dirty state tests from the code4rena audit.
| function attack( | ||
| address payable sendRecipient, | ||
| string memory bech32Recipient | ||
| ) public { | ||
| bool isSent = sendRecipient.send(10 ether); // 10 NIBI | ||
| require(isSent, "Failed to send ether"); | ||
|
|
||
| (bool success, ) = FUNTOKEN_PRECOMPILE_ADDRESS.call( | ||
| abi.encodeWithSignature( | ||
| "sendToBank(address,uint256,string)", | ||
| erc20, | ||
| uint256(10e6), // 10 WNIBI | ||
| bech32Recipient | ||
| ) | ||
| ); | ||
|
|
||
| require(success, string.concat("Failed to call bankSend")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add reentrancy protection and follow checks-effects-interactions pattern.
The function is vulnerable to reentrancy attacks as it performs external calls without proper protection.
+ bool private locked;
+
+ modifier nonReentrant() {
+ require(!locked, "Reentrant call");
+ locked = true;
+ _;
+ locked = false;
+ }
- function attack(
+ function attack nonReentrant(
address payable sendRecipient,
string memory bech32Recipient
) public {Committable suggestion skipped: line range outside the PR's diff.
| (bool success, ) = FUNTOKEN_PRECOMPILE_ADDRESS.call( | ||
| abi.encodeWithSignature( | ||
| "sendToBank(address,uint256,string)", | ||
| erc20, | ||
| uint256(10e6), // 10 WNIBI | ||
| bech32Recipient | ||
| ) | ||
| ); | ||
|
|
||
| require(success, string.concat("Failed to call bankSend")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for the precompile call.
The low-level call should include the revert reason in the error message for better debugging.
- (bool success, ) = FUNTOKEN_PRECOMPILE_ADDRESS.call(
+ (bool success, bytes memory data) = FUNTOKEN_PRECOMPILE_ADDRESS.call(
abi.encodeWithSignature(
"sendToBank(address,uint256,string)",
erc20,
uint256(10e6), // 10 WNIBI
bech32Recipient
)
);
- require(success, string.concat("Failed to call bankSend"));
+ require(success, string.concat("Failed to call bankSend: ", data.length > 0 ? string(data) : "unknown error"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (bool success, ) = FUNTOKEN_PRECOMPILE_ADDRESS.call( | |
| abi.encodeWithSignature( | |
| "sendToBank(address,uint256,string)", | |
| erc20, | |
| uint256(10e6), // 10 WNIBI | |
| bech32Recipient | |
| ) | |
| ); | |
| require(success, string.concat("Failed to call bankSend")); | |
| (bool success, bytes memory data) = FUNTOKEN_PRECOMPILE_ADDRESS.call( | |
| abi.encodeWithSignature( | |
| "sendToBank(address,uint256,string)", | |
| erc20, | |
| uint256(10e6), // 10 WNIBI | |
| bech32Recipient | |
| ) | |
| ); | |
| require(success, string.concat("Failed to call bankSend: ", data.length > 0 ? string(data) : "unknown error")); |
| bool isSent = sendRecipient.send(10 ether); // 10 NIBI | ||
| require(isSent, "Failed to send ether"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using transfer() instead of send() for better security.
The send() function is limited to 2300 gas which may fail in certain conditions. Using transfer() is safer as it will revert automatically on failure.
- bool isSent = sendRecipient.send(10 ether); // 10 NIBI
- require(isSent, "Failed to send ether");
+ sendRecipient.transfer(10 ether); // 10 NIBI📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool isSent = sendRecipient.send(10 ether); // 10 NIBI | |
| require(isSent, "Failed to send ether"); | |
| sendRecipient.transfer(10 ether); // 10 NIBI |
| require( | ||
| ERC20(erc20).transfer(sendRecipient, 1e6), // 1 WNIBI | ||
| "ERC-20 transfer failed" | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add balance and allowance checks before ERC20 transfer.
The transfer may fail silently if the contract doesn't have sufficient balance or allowance.
+ require(
+ ERC20(erc20).balanceOf(address(this)) >= 1e6,
+ "Insufficient balance"
+ );
require(
ERC20(erc20).transfer(sendRecipient, 1e6), // 1 WNIBI
"ERC-20 transfer failed"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require( | |
| ERC20(erc20).transfer(sendRecipient, 1e6), // 1 WNIBI | |
| "ERC-20 transfer failed" | |
| ); | |
| require( | |
| ERC20(erc20).balanceOf(address(this)) >= 1e6, | |
| "Insufficient balance" | |
| ); | |
| require( | |
| ERC20(erc20).transfer(sendRecipient, 1e6), // 1 WNIBI | |
| "ERC-20 transfer failed" | |
| ); |
| (bool success, ) = FUNTOKEN_PRECOMPILE_ADDRESS.call( | ||
| abi.encodeWithSignature( | ||
| "sendToBank(address,uint256,string)", | ||
| erc20, | ||
| uint256(9e6), // 9 WNIBI | ||
| bech32Recipient | ||
| ) | ||
| ); | ||
|
|
||
| require(success, string.concat("Failed to call sendToBank")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve precompile call error handling and use SafeERC20.
Similar to TestDirtyStateAttack1, improve error handling and consider using OpenZeppelin's SafeERC20.
+import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+
contract TestDirtyStateAttack2 {
+ using SafeERC20 for IERC20;
address erc20;Committable suggestion skipped: line range outside the PR's diff.
| constructor(address erc20_) payable { | ||
| erc20 = erc20_; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add zero address validation in constructor.
The contract should validate that the provided ERC20 token address is not zero to prevent potential issues.
constructor(address erc20_) payable {
+ require(erc20_ != address(0), "Zero address not allowed");
erc20 = erc20_;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(address erc20_) payable { | |
| erc20 = erc20_; | |
| } | |
| constructor(address erc20_) payable { | |
| require(erc20_ != address(0), "Zero address not allowed"); | |
| erc20 = erc20_; | |
| } |
| address payable sendRecipient, | ||
| string memory bech32Recipient | ||
| ) external { | ||
| require(sendRecipient.send(10 ether), "ETH transfer failed"); // 10 NIBI |
There was a problem hiding this comment.
Replace deprecated send() with call().
The send() function is deprecated and limited to 2300 gas which may be insufficient for token transfers. This can lead to failed transactions in certain conditions.
- require(sendRecipient.send(10 ether), "ETH transfer failed"); // 10 NIBI
+ (bool sent, ) = sendRecipient.call{value: 10 ether}("");
+ require(sent, "ETH transfer failed"); // 10 NIBI📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require(sendRecipient.send(10 ether), "ETH transfer failed"); // 10 NIBI | |
| (bool sent, ) = sendRecipient.call{value: 10 ether}(""); | |
| require(sent, "ETH transfer failed"); // 10 NIBI |
| @@ -137,7 +137,7 @@ func TestGetCirculatingSupplyAndInflationRate(t *testing.T) { | |||
| }{ | |||
| { | |||
| "no epochs per period", | |||
| sdk.TokensFromConsensusPower(400_000_000, sdk.DefaultPowerReduction), | |||
| sdk.TokensFromConsensusPower(400_000_000-100_000_001, sdk.DefaultPowerReduction), | |||
There was a problem hiding this comment.
Have to account for the new tokens from genesis state that were changed from stake to unibi.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2176 +/- ##
==========================================
- Coverage 65.01% 64.95% -0.07%
==========================================
Files 277 277
Lines 22343 22367 +24
==========================================
+ Hits 14527 14529 +2
- Misses 6823 6845 +22
Partials 993 993
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/evm/embeds/embeds.go (1)
161-170: Enhance contract documentation with security context.While the comments describe the basic functionality, they could be more detailed about:
- The specific security vulnerabilities being tested
- The expected vs. malicious behavior
- For TestDirtyStateAttack5, avoid hardcoding the amount "5 NIBI" in the comment as it may change
Example improved documentation:
- // SmartContract_TestDirtyStateAttack4 is a test contract that composes manual send and funtoken sendToBank with a reversion + // SmartContract_TestDirtyStateAttack4 is a test contract that validates state consistency + // during complex interactions. It attempts to exploit potential state inconsistencies by + // combining manual send operations with funtoken's sendToBank followed by a reversion, + // which could lead to fund duplication if not handled properly. - // SmartContract_TestDirtyStateAttack5 is a test contract that calls a wasm contract with 5 NIBI + // SmartContract_TestDirtyStateAttack5 is a test contract that validates cross-VM state handling + // by attempting to exploit potential state inconsistencies in EVM to Wasm VM interactions + // through token transfers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/evm/embeds/embeds.go(3 hunks)x/evm/keeper/funtoken_from_coin_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/keeper/funtoken_from_coin_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (3)
x/evm/embeds/embeds.go (3)
50-53: LGTM! Variable declarations follow established patterns.The new embedded contract variables are properly declared and follow the existing naming conventions.
190-191: LGTM! Proper initialization of new contracts.The new contracts are correctly initialized using MustLoad() following the established pattern.
161-170: Verify the implementation of security test contracts.Since these contracts are testing security-critical scenarios, let's verify their implementations:
✅ Verification successful
Security test contracts correctly implement their intended purposes
- TestDirtyStateAttack4 properly implements state tracking and 1 NIBI transfer for testing reversion scenarios
- TestDirtyStateAttack5 correctly implements the 5 NIBI transfer to WASM contract as described
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of dirty state attack test contracts # Check the contract implementations echo "Checking TestDirtyStateAttack4 implementation:" cat x/evm/embeds/contracts/TestDirtyStateAttack4.sol echo -e "\nChecking TestDirtyStateAttack5 implementation:" cat x/evm/embeds/contracts/TestDirtyStateAttack5.sol # Verify that the contracts are properly compiled echo -e "\nVerifying contract artifacts:" jq '.abi' x/evm/embeds/artifacts/contracts/TestDirtyStateAttack4.sol/TestDirtyStateAttack4.json > /dev/null jq '.abi' x/evm/embeds/artifacts/contracts/TestDirtyStateAttack5.sol/TestDirtyStateAttack5.json > /dev/nullLength of output: 2057
Add more EVM testing and small refactors.
Dirty state attacks can be found at https://www.notion.so/nibiru/Dirty-state-attack-126d2597a03c8075ba1bee9e86d607fd?pvs=4.