Skip to content

m-03-audit: ensure to send only msg.value and not balance#85

Merged
thelostone-mc merged 1 commit intoaudit-fixesfrom
m-03-fix
Aug 28, 2025
Merged

m-03-audit: ensure to send only msg.value and not balance#85
thelostone-mc merged 1 commit intoaudit-fixesfrom
m-03-fix

Conversation

@thelostone-mc
Copy link
Copy Markdown
Contributor

@thelostone-mc thelostone-mc commented Aug 27, 2025

Based on feedback

The balance of the whole contract is being forwarded with address(this).balance. Since this is intended to only forward the balance sent, using msg.value could result in a more deterministic and safer flow.

Copy link
Copy Markdown
Member

@apbendi apbendi left a comment

Choose a reason for hiding this comment

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

Looks good @thelostone-mc

Base automatically changed from timelock-eth-test to audit-fixes August 28, 2025 05:39
@github-actions
Copy link
Copy Markdown

Coverage after merging m-03-fix into audit-fixes will be

100.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   RollbackManager.sol100%100%100%100%
   RollbackManagerTimelockCompound.sol100%100%100%100%
   RollbackManagerTimelockControl.sol100%100%100%100%
   TimelockMultiAdminShim.sol100%100%100%100%

@thelostone-mc thelostone-mc merged commit a2d3010 into audit-fixes Aug 28, 2025
6 checks passed
@thelostone-mc thelostone-mc deleted the m-03-fix branch August 28, 2025 05:52
) public payable nonReentrant returns (bytes memory) {
_revertIfNotAdminOrExecutor();
return TIMELOCK.executeTransaction{value: address(this).balance}(_target, _value, _signature, _data, _eta);
return TIMELOCK.executeTransaction{value: msg.value}(_target, _value, _signature, _data, _eta);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Basically OZ is suggesting we use _value instead of msg.value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants