MultiSig Timelock
First Flight #55
Beginner FriendlyWallet
100 EXP
View all submissions
Previous Next
Submission Details
Impact: low
Likelihood: medium

Revoke Signer Gas Optimization

Author Revealed upon completion

Revoking Signer Role Uses More Gas Than Necessary

Description

  • When the owner revokes a users signer role, the revokeSigningRole function iterates over s_signerCount, a storage variable. Because this variable needs to be read every iteration, each iteration of the loop increases the gas used by the function.

  • This can be mitigated by read the storage variable once and assigning it to a local variable. Thus, the function will iterate of the local variable, decreasing the number of storage reads and gas consumed.


function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max; // Use max as "not found" indicator
for (uint256 i = 0; i < s_signerCount; i++) { // ITERATES OVER s_signerCount, A STORAGE VARIABLE
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
// Move the last signer to the position of the removed signer
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}

Risk

Likelihood:

  • This will occur anytime the owner wants to revoke a users signer role.

Impact:

  • Low, will use more gas than necessary.

Proof of Concept

  • This test function was run against the current revokeSigningRole function and the proposed revokeSigningRole function.

  • The current implementation used 269047 gas and the proposed implementation used 268647 gas.

function testRevokeGasOptimization() public {
vm.prank(OWNER);
multiSigTimelock.grantSigningRole(SIGNER_TWO);
multiSigTimelock.grantSigningRole(SIGNER_THREE);
multiSigTimelock.grantSigningRole(SIGNER_FOUR);
multiSigTimelock.grantSigningRole(SIGNER_FIVE);
​
​
multiSigTimelock.revokeSigningRole(SIGNER_FIVE);
}

Recommended Mitigation

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max; // Use max as "not found" indicator
+ uint256 count = s_signerCount;
- for (uint256 i = 0; i < s_signerCount; i++) { // Gas: 269047
+ for (uint256 i = 0; i < count; i++) { // Gas: 268647
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
// Move the last signer to the position of the removed signer
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!