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

Any gang member can remove any gang member

r4bbit

Summary

Due to a missing check in Laundrette::quiteTheGang, any gang member is allowed to remove any other gang member. Gang members should only be able to remove themselves. The only account that is allowed to remove other gang members should be the godfather.

Vulnerability Details

The function Laundrette::quitTheGang allows for revoking the gangmember role from accounts. This function can be called by any other gang member as ensured by the onlyRole modifier:

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

While it makes sense for individual gang members to remove themselves from the system by calling Laundrette::quitTheGang with their own account as account parameter, it should not be possible to remove any other gang member.

If at all, only the godfather should be allowed to remove the role from accounts other than his own.

Impact

Gang members that are eligible to receive weapens from the WeaponShelf will be unable to withdraw their weapons via Laundrette::takeGuns, if any other gang member removed their role.

This breaks a core purpose of the protocol.

Another scenario is that an account front-runs any call made by the godfather to Laundrette::addToTheGang and remove his gangmember role, preventing him from adding new members to the gang. This is actually a bug of itself covered in a different report, this function should not require the godfather to be a gang member in the first place. However, if for whatever reason that bug is not addressed, then this remains a problem.

Tools Used

  • Manual review

  • Foundry for testing

Recommended Mitigation

To ensure only the godfather can remove other gang members, and gang members can only remove themselves, we can make use of the already existing isAuthorizedOrRevert modifier in combination with the onlyRole modifier, similar to how it's done in Laundrette::takeGuns:

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

Proof of Concept

Below is a test that shows how an arbitrary gang member can remove the gangmember role from any other gang member. It can be dropped right into Laundrette.t.sol.

+ function test_AnyGangMemberCanRemoveGangMembers() public {
+
+ address gangMemberOne = makeAddr("gangMemberOne");
+ address gangMemberTwo = makeAddr("gangMemberTwo");
+
+ // ensure `gangMemberOne` and `gangMemberTwo` are gang members
+ vm.startPrank(address(laundrette));
+
+ kernel.grantRole(Role.wrap("gangmember"), gangMemberOne);
+ kernel.grantRole(Role.wrap("gangmember"), gangMemberTwo);
+
+ assert(kernel.hasRole(gangMemberOne, Role.wrap("gangmember")));
+ assert(kernel.hasRole(gangMemberTwo, Role.wrap("gangmember")));
+ vm.stopPrank();
+
+ // `gangMemberOne` removes `gangMemberTwo` from the gang
+ vm.prank(gangMemberOne);
+ laundrette.quitTheGang(gangMemberTwo);
+
+ // ensure `gangMemberTwo` is no longer a gang member
+ assert(!kernel.hasRole(gangMemberTwo, Role.wrap("gangmember")));
+
+ // `gangMemberTwo` tries to withdraw weapons but can't
+ vm.prank(gangMemberTwo);
+ vm.expectRevert(abi.encodeWithSignature("Policy_OnlyRole(bytes32)", Role.wrap("gangmember")));
+ laundrette.takeGuns(gangMemberTwo, 1);
+ }
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.