code-423n4 / 2023-09-delegate-findings

2 stars 1 forks source link

the enable parameter in the delegate* functions does not always behave as expected #233

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/DelegateRegistry.sol#L48 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/DelegateRegistry.sol#L56

Vulnerability details

Impact

If enable is passed as false when it should be true, the delegation will not be disabled as intended.

Proof of Concept

The potential vulnerability here is that the enable parameter in the delegate* functions does not always behave as expected. Specifically: • If enable is true, it will enable the delegation as expected by writing the msg.sender to the storage slot for the delegation. • However, if enable is false, it will only disable the delegation if the msg.sender matches the from address stored for that delegation. This means if enable is passed as false when it should be true, the delegation will not be disabled as intended. A proof of concept exploit would be:

  1. Alice enables a delegation to Bob
  2. Eve calls delegateAll with enable = false trying to disable Alice's delegation
  3. The enable check passes, but the loadedFrom != msg.sender check fails
  4. The delegation remains enabled from Alice to Bob

Tools Used

Manual

Recommended Mitigation Steps

delegate* functions could be refactored to pass a separate enabled parameter instead of overloading the enable parameter. Explicitly check enabled == true to enable and enabled == false to disable. Remove the msg.sender check for disabling

Assessed type

Other

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid