search

code-423n4 / 2022-08-foundation-findings

0 stars 0 forks source link

`MarketFees`'s `treasury` can have potentially a malicious admin #195

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df42/contracts/FoundationTreasury.sol#L66-L68

Vulnerability details

Impact / Proof of Concept

In contracts/FoundationTreasury.sol, an attacker can frontrun a call to initialize to register as an admin. If the address of this treasury is shared or is already shared with NFTDropMarket's constructor (line 83), then on line 87, FoundationTreasuryNode registers this treasury and MarketFees will send all the protocol fees to this treasury which can be drained by the attacker anytime. The attacker would only need to call withdrawFunds from CollateralManagement on line 36

Tools Used

N/A

Recommended Mitigation Steps

Either set an admin for contracts/FoundationTreasury.sol in its constructor or restrict access to initialize method of this contract to a certain contract or EOA. Also, make sure the admins are actually approved admins before sharing the treasury address with other contracts such as MarketFees contract.

HardlyDifficult commented 2 years ago

The treasury contract was listed as out of scope.

Additionally, the deploy scripts we use will set the proxy's implementation address and call initialize in a single transaction -- preventing the frontrun described here. Even if we were to deploy a fresh stack it should be safe.

HickupHH3 commented 2 years ago

Agree with the sponsor. Treasury's out of scope, deploy script usually handles frontrunning of initializations.