Reentrancy vulnerability in `Basket` contract's `initialize()` method.

[code423n4]

Handle

broccolirob

Vulnerability details

A malicious "publisher" can create a basket proposal that mixes real ERC20 tokens with a malicious ERC20 token containing a reentrancy callback in it's approve() method. When the initialize() method is called on the newly cloned Basket contract, a method called approveUnderlying(address(auction)) is called, which would trigger the reentrancy, call initialize() again, passing in altered critical values such as auction and factory, and then removes its self from proposal.tokens and proposal.weights so it doesn't appear in the token list to basket users.

https://github.com/code-423n4/2021-12-defiprotocol/blob/main/contracts/contracts/Basket.sol#L44-L61

Impact

Auction and Factory can be set to custom implementations that do malicious things. Since all baskets and auctions are clones with their own addresses, this fact would be difficult for users to detect. Auction controls ibRatio, which a malicious version could send back a manipulated value to Basket, allowing the malicious "publisher" to burn basket tokens till all users underlying tokens are drained.

Tools Used

Manual review and Hardhat.

Recommended Mitigation Steps

Since Basket inherits from ERC20Upgradeable the initializer modifier should be available and therefore used here. It has an inititializing variable that would prevent this kind of reentrancy attack.

[0xleastwood]

While the warden is correct, a malicious publisher could re-enter the Basket.initialize() function and overwrite factory and auction with their own addresses, this does not lead to a direct loss of funds for users. It would require that users interact with their malicious contracts which is entirely possible if baskets created via the factory are deemed as trusted. I think this fits the criteria of a medium severity issue.