test(evm): statedb tests for race conditions within funtoken precompile #2098
Conversation
WalkthroughThe pull request introduces several updates across various components of the Nibiru project, particularly focusing on the Nibiru EVM. Key changes include enhancements to the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
x/evm/embeds/embeds_test.go (2)
19-19: Inconsistent naming convention detected.
The contract name SmartContract_TestNativeSendThenPrecompileSendJson includes a Json suffix which is inconsistent with other contract naming patterns in the test. Consider renaming to maintain consistency.
- embeds.SmartContract_TestNativeSendThenPrecompileSendJson.MustLoad()
+ embeds.SmartContract_TestNativeSendThenPrecompileSend.MustLoad()19-20: LGTM! Consider enhancing test coverage.
The addition of these contract loads aligns well with the PR objectives to test race conditions in the funtoken precompile. The test appropriately verifies that the contracts can be loaded without panicking.
Consider adding additional test cases to verify the actual bytecode content of these contracts, ensuring they match expected values.
Example enhancement:
func TestContractBytecode(t *testing.T) {
got := embeds.SmartContract_TestNativeSendThenPrecompileSend.Load()
require.NotEmpty(t, got, "contract bytecode should not be empty")
// Add more specific bytecode validation if needed
}x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol (2)
1-9: Consider pinning the Solidity version for test consistency.
The caret (^) in the version pragma allows any version >= 0.8.24 but < 0.9.0. For test contracts, it's better to pin to a specific version to ensure consistent behavior.
-pragma solidity ^0.8.24;
+pragma solidity 0.8.24;17-38: Enhance race condition testing capabilities.
The current sequential implementation might not effectively capture all race condition scenarios. Consider the following improvements:
- Add events to track operation order
- Use try/catch instead of require for better error details
- Consider adding a delay mechanism or state check between operations
contract TestERC20TransferThenPrecompileSend {
+ event ERC20TransferCompleted(address recipient, uint256 amount);
+ event PrecompileCallCompleted(string recipient, uint256 amount);
+
// ... existing code ...
function erc20TransferThenPrecompileSend(
address payable transferRecipient,
uint256 transferAmount,
string memory precompileRecipient,
uint256 precompileAmount
) public {
- require(
- ERC20(erc20).transfer(transferRecipient, transferAmount),
- "ERC-20 transfer failed"
- );
+ bool success = ERC20(erc20).transfer(transferRecipient, transferAmount);
+ require(success, "ERC-20 transfer failed");
+ emit ERC20TransferCompleted(transferRecipient, transferAmount);
- (bool success, ) = FUNTOKEN_PRECOMPILE_ADDRESS.call(
+ (bool callSuccess, bytes memory result) = FUNTOKEN_PRECOMPILE_ADDRESS.call(
abi.encodeWithSignature(
"bankSend(address,uint256,string)",
erc20,
uint256(precompileAmount),
- precompileRecipient
+ precompileRecipient
)
);
- require(success, string.concat("Failed to call bankSend"));
+ require(callSuccess, string.concat("Failed to call bankSend: ", _decodeError(result)));
+ emit PrecompileCallCompleted(precompileRecipient, precompileAmount);
}
+
+ function _decodeError(bytes memory result) internal pure returns (string memory) {
+ if (result.length == 0) return "unknown error";
+ return string(result);
+ }
}x/evm/precompile/funtoken.go (2)
Line range hint 82-83: Incorrect use of 'sync.Mutex' with a non-existent 'TryLock' method
The sync.Mutex type in Go's standard library does not have a TryLock method. Attempting to call TryLock on a sync.Mutex will result in a compilation error.
Apply this diff to fix the issue by using Lock and handling potential blocking:
var executionGuard sync.Mutex
func (p precompileFunToken) bankSend(
start OnRunStartResult,
caller gethcommon.Address,
readOnly bool,
) (bz []byte, err error) {
- if !executionGuard.TryLock() {
- return nil, fmt.Errorf("bankSend is already in progress")
- }
+ executionGuard.Lock()
defer executionGuard.Unlock()Note: Using Lock will block until the mutex is available, which might impact performance if bankSend is called concurrently. Consider whether serializing all bankSend operations is acceptable for your application's performance and concurrency requirements.
Also applies to: 94-97
Line range hint 145-145: Typo in error messages: Remove extra 'for'
The error messages contain a grammatical error. The phrase "type validation for failed for" should be corrected to "type validation failed for".
Apply this diff to correct the error messages:
if e := assertNumArgs(len(args), 3); e != nil {
err = e
return
}
-err = fmt.Errorf("type validation for failed for (address erc20) argument")
+err = fmt.Errorf("type validation failed for (address erc20) argument")
erc20, ok := args[0].(gethcommon.Address)
if !ok {
return
}
-err = fmt.Errorf("type validation for failed for (uint256 amount) argument")
+err = fmt.Errorf("type validation failed for (uint256 amount) argument")
amount, ok = args[1].(*big.Int)
if !ok {
return
}
-err = fmt.Errorf("type validation for failed for (string to) argument")
+err = fmt.Errorf("type validation failed for (string to) argument")
to, ok = args[2].(string)
if !ok {
return
}Also applies to: 150-150, 155-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- CHANGELOG.md (1 hunks)
- x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json (1 hunks)
- x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json (1 hunks)
- x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json (1 hunks)
- x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol (1 hunks)
- x/evm/embeds/contracts/TestNativeSendThenPrecompileSend.sol (1 hunks)
- x/evm/embeds/embeds.go (5 hunks)
- x/evm/embeds/embeds_test.go (1 hunks)
- x/evm/keeper/funtoken_from_coin_test.go (2 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json
- x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json
- x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json
🔇 Additional comments (14)
x/evm/embeds/contracts/TestNativeSendThenPrecompileSend.sol (1)
14-33:
Critical: Multiple security concerns in transaction handling.
Several security issues need to be addressed:
-
Using
send()is discouraged as it:- Only forwards 2300 gas which might be insufficient
- Silently fails instead of reverting
-
The function is vulnerable to reentrancy as it makes external calls without following checks-effects-interactions pattern
-
Since this is a test for race conditions, it should include comments documenting the expected behavior and potential vulnerabilities being tested
Apply these fixes:
function nativeSendThenPrecompileSend(
address payable nativeRecipient,
uint256 nativeAmount,
string memory precompileRecipient,
uint256 precompileAmount
- ) public {
- bool isSent = nativeRecipient.send(nativeAmount);
- require(isSent, "Failed to send native token");
+ ) public payable {
+ // Test Case: This function tests for potential race conditions where state DB
+ // commit could overwrite state after precompile execution leading to fund loss
+
+ require(msg.value >= nativeAmount, "Insufficient native tokens");
+ (bool success, ) = nativeRecipient.call{value: nativeAmount}("");
+ require(success, "Native token transfer failed");
(bool success, ) = FUNTOKEN_PRECOMPILE_ADDRESS.call(
abi.encodeWithSignature(
"bankSend(address,uint256,string)",
erc20,
precompileAmount,
precompileRecipient
)
);
- require(success, string.concat("Failed to call precompile bankSend"));
+ require(success, "Precompile bankSend failed");
}Additionally, consider adding:
- A
nonReentrantmodifier from OpenZeppelin - Events to track test execution
- More detailed error messages including amounts
✅ Verification successful
Critical: Replace send() with call() for safer value transfers
The verification confirms two key findings:
-
The
send()pattern is used in two contracts:TestNativeSendThenPrecompileSend.solSendReceiveNibi.sol
-
SendReceiveNibi.solalready uses the safer.call{value: amount}("")pattern, which should be adopted inTestNativeSendThenPrecompileSend.solas well.
The original review recommendations remain valid:
- Replace
send()withcall()for native token transfers - Add test documentation since this is a race condition test
- Use more descriptive error messages
However, the reentrancy concern can be relaxed since:
- No reentrancy protection was found in the codebase, suggesting it's not part of the testing requirements
- This is a test contract specifically for race conditions, where the interaction pattern is intentional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are similar patterns in other test contracts
rg "send\(" --type solidity
Length of output: 62
Script:
#!/bin/bash
# Search for .sol files first to confirm file extension
fd -e sol
# Then search for send patterns in those files
rg "\.send\("
# Also search for similar call patterns
rg "\.call\{"
# Look for reentrancy protection
rg -i "nonreentrant|reentrancy"
Length of output: 4459
x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol (2)
10-12: LGTM!
The contract state is minimal and appropriate for its testing purpose.
17-38: Verify state consistency between operations.
To effectively test for race conditions that could lead to fund loss or infinite minting, consider adding balance checks between operations.
function erc20TransferThenPrecompileSend(
address payable transferRecipient,
uint256 transferAmount,
string memory precompileRecipient,
uint256 precompileAmount
) public {
+ uint256 initialBalance = ERC20(erc20).balanceOf(transferRecipient);
+ uint256 initialSupply = ERC20(erc20).totalSupply();
+
require(
ERC20(erc20).transfer(transferRecipient, transferAmount),
"ERC-20 transfer failed"
);
+ uint256 postTransferBalance = ERC20(erc20).balanceOf(transferRecipient);
+ require(
+ postTransferBalance == initialBalance + transferAmount,
+ "Unexpected balance after transfer"
+ );
+
// ... precompile call ...
+
+ uint256 finalSupply = ERC20(erc20).totalSupply();
+ require(
+ finalSupply == initialSupply,
+ "Total supply changed unexpectedly"
+ );
}x/evm/embeds/embeds.go (4)
22-22: LGTM! The contract path changes and new test contracts align with the PR objectives.
The path change from FunToken.sol to IFunToken.sol reflects a good practice of using interfaces. The new test contract embeds are well-documented and specifically target the race conditions mentioned in the PR objectives.
Also applies to: 32-35
47-50: Well-documented test contracts for race condition scenarios.
The test contracts are well-structured and their comments clearly document the race conditions being tested:
TestNativeSendThenPrecompileSend: Tests potential fund loss during state DB commitTestERC20TransferThenPrecompileSend: Tests potential infinite token mint during state DB commit
These align perfectly with the PR objectives.
Also applies to: 83-98
83-98: Verify test coverage for race conditions.
The test contracts are well-designed to catch critical race conditions. Let's verify they are properly utilized in test files.
✅ Verification successful
Let me check the actual test implementations to verify they properly test the race conditions.
Let me check the actual test contract implementations to verify they properly test the race conditions mentioned in the comments.
Test coverage for race conditions is properly implemented
The verification confirms:
- Both test contracts are properly implemented to test the race conditions:
TestNativeSendThenPrecompileSendtests native send followed by precompile bankSendTestERC20TransferThenPrecompileSendtests ERC20 transfer followed by precompile bankSend
- Test functions in
funtoken_from_coin_test.goexecute these contracts with proper setup and assertions - The test implementations align with the documented race condition scenarios in the comments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the test contracts are referenced in test files
# and that there are corresponding test cases.
echo "Searching for test references..."
rg -l "TestNativeSendThenPrecompileSend|TestERC20TransferThenPrecompileSend" --type go
echo "Checking for test functions..."
rg "func.*(TestNativeSendThenPrecompileSend|TestERC20TransferThenPrecompileSend)" --type go
Length of output: 640
Script:
#!/bin/bash
# Check the implementation of the test functions
rg -A 20 "func.*TestNativeSendThenPrecompileSend|func.*TestERC20TransferThenPrecompileSend" x/evm/keeper/funtoken_from_coin_test.go
Length of output: 1432
Script:
#!/bin/bash
# Search for the test contract implementations
rg -l "TestNativeSendThenPrecompileSend\.sol|TestERC20TransferThenPrecompileSend\.sol" --type-add 'sol:*.sol'
echo "Checking contract contents if found:"
fd "TestNativeSendThenPrecompileSend\.sol|TestERC20TransferThenPrecompileSend\.sol" -x cat {}
Length of output: 2788
109-110:
Fix naming inconsistency in contract initialization.
The variable name in the initialization differs from its declaration:
- Declaration:
SmartContract_TestNativeSendThenPrecompileSendJson - Usage: Same variable name with
Jsonsuffix
This should be consistent across the codebase.
Apply this diff to fix the naming:
- SmartContract_TestNativeSendThenPrecompileSendJson.MustLoad()
+ SmartContract_TestNativeSendThenPrecompileSend.MustLoad()Likely invalid or redundant comment.
CHANGELOG.md (1)
72-72: LGTM! The changelog entry is well-formatted and accurately describes the changes.
The entry properly documents the addition of statedb tests for race conditions within the funtoken precompile, matching the PR objectives. It follows the changelog format guidelines and includes the correct PR reference.
x/evm/precompile/funtoken.go (1)
26-28: Updated reference to 'IFunToken.sol' is appropriate
The comment now correctly references 'IFunToken.sol', ensuring accurate documentation of the contract interface being used.
x/evm/keeper/funtoken_from_coin_test.go (5)
270-361: Well-structured test for race condition in TestNativeSendThenPrecompileSend
The newly added test method effectively simulates a scenario where a native NIBI send and a precompile bankSend are performed within a single call. This is valuable for identifying potential race conditions that could lead to fund loss due to state DB commit issues.
334-336: Verify amount conversions and decimal handling
The amounts used for the native EVM send (10e6 converted with evm.NativeToWei) and the precompile bankSend (big.NewInt(10e6)) rely on correct decimal handling. Ensure that the conversion functions and decimals align with the token specifications to prevent discrepancies.
363-440: Effective test for infinite mint scenario in TestERC20TransferThenPrecompileSend
This new test method addresses the critical case of potential infinite minting due to state inconsistencies after ERC20 transfers and precompile calls. It enhances the test suite by safeguarding against severe bugs related to token minting.
378-440: Consistency in decimal representation
Throughout the test, amounts like 1e6 and 9e6 are used with the assumption of 6 decimal places. Verify that this decimal representation is consistent with the ERC20 token's Decimals setting to ensure accurate calculations and transfers.
442-479: Improved code maintainability with fundAndCreateFunToken helper
The introduction of the fundAndCreateFunToken helper function simplifies the test setup by encapsulating the common steps for funding accounts and creating fun tokens. This enhances code readability and reduces duplication.
| constructor(address erc20_) { | ||
| erc20 = erc20_; | ||
| } |
There was a problem hiding this comment.
Add input validation in constructor.
The constructor should validate that erc20_ is not the zero address to prevent potential misconfiguration.
Apply this diff to add validation:
constructor(address erc20_) {
+ require(erc20_ != address(0), "TestNativeSendThenPrecompileSend: zero address");
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_) { | |
| erc20 = erc20_; | |
| } | |
| constructor(address erc20_) { | |
| require(erc20_ != address(0), "TestNativeSendThenPrecompileSend: zero address"); | |
| erc20 = erc20_; | |
| } |
| contract TestNativeSendThenPrecompileSend { | ||
| address erc20; | ||
|
|
There was a problem hiding this comment.
Fix missing declarations and improve state variable immutability.
There are several issues to address:
- The
FUNTOKEN_PRECOMPILE_ADDRESSis used but not declared - The
erc20address should be declared asimmutablesince it's only set in constructor
Apply this diff to fix the issues:
contract TestNativeSendThenPrecompileSend {
- address erc20;
+ address public immutable FUNTOKEN_PRECOMPILE_ADDRESS = address(0x69); // Replace with actual address
+ address public immutable 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.
| contract TestNativeSendThenPrecompileSend { | |
| address erc20; | |
| contract TestNativeSendThenPrecompileSend { | |
| address public immutable FUNTOKEN_PRECOMPILE_ADDRESS = address(0x69); // Replace with actual address | |
| address public immutable erc20; | |
| // SPDX-License-Identifier: UNLICENSED | ||
| pragma solidity ^0.8.24; | ||
|
|
||
| import "./IFunToken.sol"; | ||
| import "@openzeppelin/contracts/utils/Strings.sol"; | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
The IFunToken import can be safely removed
The contract doesn't use any functionality from the IFunToken interface. It only uses low-level calls (call) to interact with the precompile address and doesn't need the interface definition. The import statement can be safely removed.
🔗 Analysis chain
Consider using MIT license for test contracts.
The UNLICENSED identifier might be too restrictive for test contracts. Consider using MIT license which is more common for test code in blockchain projects.
Also, the IFunToken import appears to be unused as the contract doesn't directly implement or use the interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if IFunToken interface is used elsewhere in the codebase
rg -l "IFunToken" --type solidity
Length of output: 66
Script:
#!/bin/bash
# Let's try searching for IFunToken without file type restriction
rg -l "IFunToken"
# Also search for actual interface definition
rg "interface IFunToken"
Length of output: 390
Script:
#!/bin/bash
# Let's check the content of TestNativeSendThenPrecompileSend.sol to see how IFunToken is used
cat x/evm/embeds/contracts/TestNativeSendThenPrecompileSend.sol
Length of output: 1014
| constructor(address erc20_) { | ||
| erc20 = erc20_; | ||
| } |
There was a problem hiding this comment.
Add zero address validation in constructor.
To prevent test setup with an invalid token address, add a zero address check.
constructor(address erc20_) {
+ require(erc20_ != address(0), "Invalid token address");
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_) { | |
| erc20 = erc20_; | |
| } | |
| constructor(address erc20_) { | |
| require(erc20_ != address(0), "Invalid token address"); | |
| erc20 = erc20_; | |
| } |
| (bool success, ) = FUNTOKEN_PRECOMPILE_ADDRESS.call( | ||
| abi.encodeWithSignature( | ||
| "bankSend(address,uint256,string)", | ||
| erc20, | ||
| uint256(precompileAmount), | ||
| precompileRecipient | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more robust precompile interface.
The current low-level call approach to the precompile is error-prone and harder to maintain. Consider creating an interface for the precompile to enable type-safe calls and better error handling.
+interface IFunTokenPrecompile {
+ function bankSend(address token, uint256 amount, string memory recipient) external returns (bool);
+}
contract TestERC20TransferThenPrecompileSend {
address erc20;
+ IFunTokenPrecompile constant FUNTOKEN_PRECOMPILE = IFunTokenPrecompile(FUNTOKEN_PRECOMPILE_ADDRESS);
// ... rest of the contract ...
- (bool success, ) = FUNTOKEN_PRECOMPILE_ADDRESS.call(
- abi.encodeWithSignature(
- "bankSend(address,uint256,string)",
- erc20,
- uint256(precompileAmount),
- precompileRecipient
- )
- );
- require(success, string.concat("Failed to call bankSend"));
+ bool success = FUNTOKEN_PRECOMPILE.bankSend(erc20, precompileAmount, precompileRecipient);
+ require(success, "Precompile bankSend failed");Committable suggestion was skipped due to low confidence.
| deps := evmtest.NewTestDeps() | ||
| bankDenom := evm.EVMBankDenom | ||
|
|
||
| // Initial setup | ||
| funTokenErc20Addr := s.fundAndCreateFunToken(deps, 10e6) | ||
|
|
||
| s.T().Log("Deploy Test Contract") | ||
| deployResp, err := evmtest.DeployContract( | ||
| &deps, | ||
| embeds.SmartContract_TestNativeSendThenPrecompileSendJson, | ||
| funTokenErc20Addr.Address, | ||
| ) | ||
| s.Require().NoError(err) | ||
|
|
||
| testContractAddr := deployResp.ContractAddr | ||
| testContractNibiAddr := eth.EthAddrToNibiruAddr(testContractAddr) | ||
|
|
||
| s.T().Log("Give the test contract 10 NIBI (native)") | ||
| s.Require().NoError(testapp.FundAccount( | ||
| deps.App.BankKeeper, | ||
| deps.Ctx, | ||
| testContractNibiAddr, | ||
| sdk.NewCoins(sdk.NewCoin(bankDenom, sdk.NewInt(10e6)))), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper funding and initialization in test setup
The test setup correctly funds the test contract with both NIBI and WNIBI. However, consider adding checks to confirm that the balances are as expected before proceeding with the contract call to enhance test reliability.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2098 +/- ##
=======================================
Coverage 64.45% 64.46%
=======================================
Files 270 270
Lines 21190 21192 +2
=======================================
+ Hits 13659 13661 +2
Misses 6579 6579
Partials 952 952
|
Notes
Introduces 2 tests of the race conditions in evm execution:
Performs two sends in a single call: a native nibi send and a precompile bankSend. It tests a race condition where the state DB commit may overwrite the state after the precompile execution, potentially causing a loss of funds.
Performs two sends in a single call: an erc20 token transfer and a precompile bankSend. It tests a race condition where the state DB commit may overwrite the state after the precompile execution, potentially causing an infinite token mint.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Refactor