search

code-423n4 / 2022-10-thegraph-findings

0 stars 0 forks source link

Using ifAdminOrPendingImpl modifier to forcefully interact with implementation contracts via _fallback() call. #279

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/upgrades/GraphProxy.sol#L33-L39 https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/upgrades/GraphProxy.sol#L170

Vulnerability details

Impact

The modifier ifAdminOrPendingImpl() allows internal delegation to the implementation contract if caller is not admin by calling the _fallback() function which delegates the current call to implementation.

This allows a user who is not admin to call to make a transaction call with data to GraphProxy functions restricted with ifAdminOrPendingImpl modifier to forcefully initialize implementation contract. and carry out activities on implementation, for example calling GraphTokenUpgradeable.permit() to approve allowance.

Proof of Concept

  1. Alice is not an admin, and sends a transaction call to GraphProxy.acceptUpgrade() with transaction data to make GraphTokenUpgradeable.permit() call
  2. Since Alice is not an admin, the call jumps to _fallback() which makes a delegation call with the transaction data to implementation contract.
  3. _fallback() makes a delegation call to GraphTokenUpgradeable.permit() to approve Alice as spender for token owner
  4. allowance is approved for Alice to spend.

Tools Used

Manual review

Recommended Mitigation Steps

The functionality of the _fallback() function may need to be revisited and/or necessarily amending the ifAdminOrPendingImpl modifiers.

Minh-Trng commented 1 year ago

dont think this would work. Proxy would try to delegatecall acceptUpgrade() on the implementation and revert because it does not exist

trust1995 commented 1 year ago

It is intentional behavior that users can call GraphTokenUpgradeable.permit()

0xean commented 1 year ago

Report is unclear on why having a user calling permit() would be unexpected behavior. Closing as invalid.