Judge has assessed an item in Issue [#769](https://github.com/code-423n4/2022-12-gogopool-findings/issues/769) as 2 risk. The relevant finding follows: [L-1] requireNextActiveMultisig() always returns the 1st enabled Multisig Relevant code: As the name suggested, MultisigManager.requireNextActiveMultisig() should return the next enabled Multisig. However, it actually always returns the 1st registered and enabled Multisig. This is because it always searches from the beginning and returns the result as long as the criteria is matched. POC: The below Testcase created 4 multisigs and the first 2 were disabled. We can see from the test that requireNextActiveMultisig() always returns the 1st enabled multsig which is rialto3. Test steps: Copy the below Testcase into test file MultisigManager.t.sol; Run the test. function testRequireNextActiveMultisigAudit() public { vm.startPrank(guardian); multisigMgr.disableMultisig(address(rialto)); address rialto1 = getActor("rialto1"); multisigMgr.registerMultisig(rialto1); multisigMgr.enableMultisig(rialto1); address rialto2 = getActor("rialto2"); multisigMgr.registerMultisig(rialto2); multisigMgr.enableMultisig(rialto2); // Disable the first two multisigMgr.disableMultisig(rialto1); multisigMgr.disableMultisig(rialto2); address rialto3 = getActor("rialto3"); multisigMgr.registerMultisig(rialto3); multisigMgr.enableMultisig(rialto3); address rialto4 = getActor("rialto4"); multisigMgr.registerMultisig(rialto4); multisigMgr.enableMultisig(rialto4); vm.stopPrank(); assertEq(rialto3, multisigMgr.requireNextActiveMultisig()); assertEq(rialto3, multisigMgr.requireNextActiveMultisig()); assertEq(rialto3, multisigMgr.requireNextActiveMultisig()); } Mitigation Steps: In order to find the next enabled Multisig, we have to know the last one used. So, we need an extra state variable, say lastMultisigIndex, to store the index of the Multisig selected last time. Refactored function: Note - the below function is optimized in terms of gas saving. File: uint256 lastMultisigIndex; //[@Audit]() This is the added state variable /// [@notice]() Gets the next registered and enabled Multisig, revert if none found /// [@dev]() There will never be more than 10 total multisigs. If we grow beyond that we will redesign this contract. function requireNextActiveMultisig() external view returns (address addr) { //[@Audit]() Use named variable returns uint256 total = getUint(keccak256("multisig.count")); uint256 lastIndex = lastMultisigIndex; //[@Audit]() Caching state variable to save gas in the below for-loop bool enabled; uint256 index; for (uint256 i; i < total;) { //[@Audit]() no need to initialize i for gas saving unchecked { //[@Audit]() It's safe to apply unchecked index = (i + lastIndex) % total; } (addr, enabled) = getMultisig(index); if (enabled) { lastMultisigIndex = index; //[@Audit]() Update state variable return addr; } unchecked { //[@Audit]() Apply unchecked to save gas as no overflow could happen ++i; //[@Audit]() ++i is more gas efficient than i++ } } revert NoEnabledMultisigFound(); } --- The text was updated successfully, but these errors were encountered: All reactions