sherlock-admin3 commented 5 months ago

Unneeded Variable Sender

Low/Info issue submitted by petarP1998

Summary

In the functions MidasAccessControl::grantRoleMult, MidasAccessControl::revokeRoleMult, and MidasAccessControl::_setupRoles, the variable sender is redundantly defined as address sender = msg.sender;. This redundancy can be removed by directly using msg.sender instead.

Vulnerability Detail

The functions grantRoleMult, revokeRoleMult, and _setupRoles in the MidasAccessControl contract unnecessarily define a local variable sender to hold the value of msg.sender. This redundant declaration does not provide any benefit and can be avoided by using msg.sender directly in the code.

https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/access/MidasAccessControl.sol#L34-L44

Impact

Increased Code Complexity: Unnecessary variables add to the complexity of the code.
Maintenance Overhead: Redundant code can cause confusion and make the codebase harder to maintain.
Potential for Errors: Additional variables increase the risk of mistakes, such as unintended modifications.

Code Snippet

function grantRoleMult(bytes32[] memory roles, address[] memory addresses)
external {
   require(roles.length == addresses.length, "MAC: mismatch arrays");

   address sender = msg.sender;
   for (uint256 i = 0; i < roles.length; i++) {
      _checkRole(getRoleAdmin(roles[i]), sender);
      _grantRole(roles[i], addresses[i]);
   }
}

Tool used

Manual Review

Recommendation

Refactor the code to remove the unnecessary sender variable and use msg.sender directly to simplify and improve code readability and maintainability.

sherlock-admin2 commented 5 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/RedDuck-Software/midas-contracts/pull/41

sherlock-admin2 commented 5 months ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-05-midas-judging