Skip to content

Commit 04edc03

Browse files
committed
m-03-audit: ensure to send only msg.value and not balance
1 parent 161c600 commit 04edc03

File tree

3 files changed

+7
-9
lines changed

3 files changed

+7
-9
lines changed

src/TimelockMultiAdminShim.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ contract TimelockMultiAdminShim is ITimelockMultiAdminShim, ReentrancyGuard {
162162
uint256 _eta
163163
) public payable nonReentrant returns (bytes memory) {
164164
_revertIfNotAdminOrExecutor();
165-
return TIMELOCK.executeTransaction{value: address(this).balance}(_target, _value, _signature, _data, _eta);
165+
return TIMELOCK.executeTransaction{value: msg.value}(_target, _value, _signature, _data, _eta);
166166
}
167167

168168
/// @inheritdoc ITimelockMultiAdminShim

test/CompoundGovernanceUpgradeImpact.integration.t.sol

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,6 @@ contract CompoundGovernanceUpgradeImpactIntegrationTest is Test, RollbackManager
247247
function _testProposalRequiringEthAndEthIsSentWithExecuteCall(uint256 _requiredEthAmount, address _executor) internal {
248248
// 0. Record initial balance for later comparison
249249
uint256 initialBalance = address(fakeProtocolContract).balance;
250-
uint256 initialTimelockBalance = COMPOUND_TIMELOCK.balance;
251250

252251
// 1. Create a proposal that requires ETH to be sent
253252
address[] memory targets = new address[](1);
@@ -274,7 +273,6 @@ contract CompoundGovernanceUpgradeImpactIntegrationTest is Test, RollbackManager
274273

275274
govHelper.executeQueuedProposal{value: _requiredEthAmount}(ethProposal);
276275
assertEq(address(fakeProtocolContract).balance, initialBalance + _requiredEthAmount);
277-
assertEq(COMPOUND_TIMELOCK.balance, initialTimelockBalance);
278276
}
279277

280278
/// @notice Test that a proposal requiring ETH works with various ETH amounts

test/TimelockMultiAdminShim.unit.t.sol

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,7 @@ contract ExecuteTransaction is TimelockMultiAdminShimTest {
509509
assertEq(timelockTxnCall.eta, _eta);
510510
}
511511

512-
function testFuzz_FlushesAccidentallySentETH(
512+
function testFuzz_OnlyForwardsIntendedETH_DoesNotFlushAccidentalETH(
513513
address _target,
514514
uint256 _value,
515515
string memory _signature,
@@ -527,15 +527,15 @@ contract ExecuteTransaction is TimelockMultiAdminShimTest {
527527
vm.deal(admin, _value);
528528
vm.prank(admin);
529529

530-
// Execute transaction - should flush ALL ETH (both the intended value and accidental ETH)
530+
// Execute transaction - should only forward the intended msg.value, not accidental ETH
531531
timelockMultiAdminShim.executeTransaction{value: _value}(_target, _value, _signature, _data, _eta);
532532

533-
// Verify that ALL ETH was flushed (both the intended value and the accidental ETH)
534-
assertEq(address(timelockMultiAdminShim).balance, 0);
533+
// Verify that accidental ETH remains in the contract (safer behavior)
534+
assertEq(address(timelockMultiAdminShim).balance, _accidentalETH);
535535

536536
// Verify that the timelock received the original intended value as the parameter
537-
// (the timelock actually receives all ETH via {value: address(this).balance},
538-
// but the _value parameter is still the original intended value)
537+
// (the timelock receives ETH via {value: msg.value},
538+
// and the _value parameter is the original intended value)
539539
MockCompoundTimelock.TimelockTransactionCall memory timelockTxnCall = timelock.lastParam__executeTransaction__();
540540
assertEq(timelockTxnCall.value, _value);
541541
}

0 commit comments

Comments
 (0)