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

2 stars 1 forks source link

The rights strings are compared directly which can lead to unintended rights being granted. #254

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#L168 https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/DelegateRegistry.sol#L181-L182

Vulnerability details

Impact

Attackers could potentially gain access to transfer or other privileged capabilities on tokens or contracts they should not have access to. This could lead to loss of funds or assets.

Proof of Concept

The key issue is in the checkDelegateForAll and checkDelegateForERC721 functions. These functions compare the passed in rights parameter directly against the stored rights value for a delegation, using an equality check:https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/DelegateRegistry.sol#L168 and https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/DelegateRegistry.sol#L181-L182 This means that if the stored rights string is "TRANSFER", and the passed in rights string is "TRANSFER_", it will evaluate to true and grant unintended rights.

An attacker could manipulate the rights string to add underscores, spaces, camelCase vs lowercase etc. to trick the contract into granting additional rights.

Tools Used

Recommended Mitigation Steps

Assessed type

Other

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof