First Flight #16: Mafia Takedown
First Flight #16
Beginner FriendlyDeFiFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

`Laundrette::quitTheGang` allows arbitrary account as argument causing unauthorized role revocation

Mill

The quitTheGang function is problematic because it allows any user with the gangmember role to revoke the role from any account, not just themselves. This can lead to unauthorized role revocation, potentially disrupting the intended access control.

Vulnerability Details

The quitTheGang function can be called with any account as an argument instead of limiting the action to msg.sender. The function allows specifying an arbitrary account for role revocation, rather than restricting the revocation to the caller msg.sender.

Proof of Code

Copy the following code into Laundrette.t.sol and then run the test:

Code
address testAccount = makeAddr("Test");
address gangMember = makeAddr("Gang Member");
​
function test_quitTheGangForOtherAddress() public {
// first we need to assign the godFather the gangmember role
// so he is allowed to call the addToTheGang function
vm.prank(kernel.admin());
kernel.grantRole(Role.wrap("gangmember"), godFather);
​
// now we can add the addresses to the gang
vm.startPrank(godFather);
laundrette.addToTheGang(testAccount);
laundrette.addToTheGang(gangMember);
vm.stopPrank();
​
assertEq(kernel.hasRole(testAccount, Role.wrap("gangmember")), true);
assertEq(kernel.hasRole(gangMember, Role.wrap("gangmember")), true);
​
// the testAccount calls quitTheGang with the gangMember address
// the result is that the gangMember is removed from the gang without permission
vm.startPrank(testAccount);
laundrette.quitTheGang(gangMember);
vm.stopPrank();
​
assertEq(kernel.hasRole(gangMember, Role.wrap("gangmember")), false);
}
forge test --mt test_quitTheGangForOtherAddress -vvv

Impact

This vulnerability permits any user with the "gangmember" role to revoke the "gangmember" role from any other account. This undermines the access control mechanism, allowing malicious users to disrupt the system by removing other users' roles without authorization.

Tools Used

  • forge test

Recommendations

There are two possible solutions to solve this issue I'd prefer number one, because the godfather has still control in this case:

  1. Remove the onlyRole("gangmember") modifier and add the isAuthorizedOrRevert(account) modifier.
    The modifier isAuthorizedOrRevert checks if the address is equal to msg.sender or if the msg.sender is the executor (godfather). We don't need the onlyRole("gangmember") anymore because the Kernel::grantRole function checks this anyways.

- function quitTheGang(address account) external onlyRole("gangmember") {
+ function quitTheGang(address account) external isAuthorizedOrRevert(account) {
kernel.revokeRole(Role.wrap("gangmember"), account);
}

  1. Restrict the quitTheGang function to only allow msg.sender to revoke their own role, but then the godfather can't call this function anymore.

- function quitTheGang(address account) external onlyRole("gangmember") {
+ function quitTheGang() external onlyRole("gangmember") {
- kernel.revokeRole(Role.wrap("gangmember"), account);
+ kernel.revokeRole(Role.wrap("gangmember"), msg.sender);
}
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Gang members ban other members

Support

FAQs

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