sherlock-audit / 2023-09-Gitcoin-judging

11 stars 7 forks source link

trachev - The Registry contract's upgradeability would not work #970

Closed sherlock-admin closed 11 months ago

sherlock-admin commented 11 months ago

trachev

medium

The Registry contract's upgradeability would not work

The Registry.sol contract is intended to be the implementation contract of a proxy contract, but the upgradeability would not be functional.

Vulnerability Detail

In Allo.sol the Registry contract is used to check whether an address is a member or owner of a profile. The issue is that calls are directly made to the Registry.sol contract. Therefore, if users create their profiles through a proxy contract that stores all of the data and delegate-calls the Registry contract, then direct calls in Allo.sol to Registry.sol would be using the wrong data, as the actual data is stored in the proxy contract. In order for the Registry to be truly upgradeable, the calls in Allo.sol need to be made to a proxy contract which would then delegate-call the implementation Registry contract.

Impact

Calls to Registry.sol in Allo.sol would retreive wrong data.

Code Snippet

https://github.com/allo-protocol/allo-v2/blob/851571c27df5c16f6586ece2a1cb6fd0acf04ec9/contracts/core/Allo.sol#L424

if (!registry.isOwnerOrMemberOfProfile(_profileId, msg.sender)) revert UNAUTHORIZED();

Tool used

Manual Review

Recommendation

Do not make direct calls to Registry.sol in Allo.sol but instead make calls to a proxy contract in order to preserve the upgradeability of the Registry contract.

sherlock-admin commented 11 months ago

1 comment(s) were left on this issue during the judging contest.

n33k commented:

invalid, Registry is called via proxy