Submission Details
Impact:
Likelihood:
Signers role cannot propose a transaction
Signers role cannot propose a transaction
Description
-
In the README of the project, we can read that signer can propose new transaction. (Propose new transactions (permission is tied to the role, so any signer can propose))
-
However, in the code implementation, only the admin can call the "proposeTransaction()" function.
function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
onlyOwner // Here: only the owner can call this function
returns (uint256)
{
Risk
Likelihood:
It will impact the protocol as it does not match the expected implementation.
Impact:
Impact the protocol behavour as here only the admin can propose new transactions.
Proof of Concept
function testSignerCanProposeTransaction() public {
// ARRANGE
vm.deal(OWNER, OWNER_BALANCE_ONE);
vm.deal(SIGNER_TWO, OWNER_BALANCE_ONE);
// ACT & ASSERT
vm.prank(OWNER);
multiSigTimelock.grantSigningRole(SIGNER_TWO);
vm.prank(SIGNER_TWO);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
console2.log("This is the first transaction ID", txnId);
assertEq(txnId, 0);
}
[108237] MultiSigTimeLockTest::testSignerCanProposeTransaction()
├─ [0] VM::deal(MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 500000000000000000 [5e17])
│ └─ ← [Return]
├─ [0] VM::deal(signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa], 500000000000000000 [5e17])
│ └─ ← [Return]
├─ [0] VM::prank(MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return]
├─ [83181] MultiSigTimelock::grantSigningRole(signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa])
│ ├─ emit RoleGranted(role: 0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b, account: signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa], sender: MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Stop]
├─ [0] VM::prank(signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa])
│ └─ ← [Return]
├─ [4789] MultiSigTimelock::proposeTransaction(spender_one: [0x488dE584674d70E8Da70C42Cd4CC73Cd63d3dB23], 500000000000000000 [5e17], 0x)
│ └─ ← [Revert] OwnableUnauthorizedAccount(0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa)
└─ ← [Revert] OwnableUnauthorizedAccount(0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa)
Recommended Mitigation
I would recommand you to use direclty the onlyRole directive and set it to the SIGNING_ROLE instead.
function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
- onlyOwner
+ onlyRole(SIGNING_ROLE)
returns (uint256)
{
Updates
Rewards breakdown
nSLOC
205This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.