[WP-H3] Imprecise management of users' allowance allows the admin of the upgradeable proxy contract to rug users #68
Labels
2 (Med Risk)
Assets not at direct risk, but function/availability of the protocol could be impacted or leak value
bug
Something isn't working
sponsor acknowledged
Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Lines of code
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/FeeFollowModule.sol#L75-L91
Vulnerability details
In the current implementation, when there is a fee on follow or collect, users need to approve to the follow modules or collect module contract, and then the
Hub
contract can callprocessFollow()
and transfer funds from an arbitrary address (as thefollower
parameter).https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/FeeFollowModule.sol#L75-L91
A common practice is asking users to approve an unlimited amount to the contracts. Normally, the
allowance
can only be used by the user who initiated the transaction.It's not a problem as long as
transferFrom
will always only transfer funds frommsg.sender
and the contract is non-upgradable.However, the
LensHub
contract is upgradeable, andFeeFollowModule
willtransferFrom
an address decided by an input parameterfollower
, use of upgradeable proxy contract structure allows the logic of the contract to be arbitrarily changed.This allows the proxy admin to perform many malicious actions, including taking funds from users' wallets up to the allowance limit.
This action can be performed by the malicious/compromised proxy admin without any restriction.
PoC
Given:
1
usesFeeCollectModule
withcurrency
=WETH
andamount
=1e18
approve()
type(uint256).max
toFeeCollectModule
andfollow()
profileId1
with1e18 WETH
;upgradeToAndCall()
on the proxy contract and set a malicious contract asnewImplementation
, adding a new functions:FeeCollectModule
and setcurrency
=WETH
andamount
=1,000,000
, got profileId =2
rugFollow()
withfollower
=Alice
, profileId =2
, stolen1,000,000
WETH from Alice's walletRecommendation
Improper management of users'
allowance
is the root cause of some of the biggest attacks in the history of DeFi security. We believe there are 2 rules that should be followed:from
oftransferFrom
should always bemsg.sender
;allowance
should always be non-upgradable.We suggest adding a non-upgradeable contract for processing user payments:
This comes with an additional benefit: users only need to approve one contract instead of approving multiple
follow
orcollect
module contracts.The text was updated successfully, but these errors were encountered: