Admin Privilege - Owner can rug via `ThecosomataETH.withdraw`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/ThecosomataETH.sol#L158

Vulnerability details

Impact

Due to the generalized nature of withdraw the function is a clear rug-vector, allowing the owner to steal all funds.

Ideally, you should add some validation logic to limit the tokens or the amounts that the owner can withdraw

Additionally, it's important that you disclose the level of admin privilege and the risk it can cause to your users and depositors

Recommended Mitigation Steps

Disclose the admin privilege in your docs Refactor the code to reduce it

[kphed]

The owner is our protocol multisig which has proven itself to be a trustworthy steward of funds (e.g. manages the Redacted treasury funds).

The withdraw method is simply a utility to remove any ERC20 tokens that are unintentionally received. There won't be any funds to steal since it's not intended for the Thecosomata contract to custody funds for any extended period of time: our keepers will constantly poll the contract so that any BTRFLY received gets paired with ETH and added to our Curve LP immediately - any excess is burned.

[GalloDaSballo]

It should be noted that I have submitted the finding, and in being judge of the contest am forfeiting my potential winnings.

Personally I don't believe a multisig gives any particular security guarantee to depositors beside the fact that it takes X amount of people to agree on how to move funds.

The sponsor is making it clear that the owner in this case is also the depositor of funds. This means that the multi-sig is self custodying the funds into the contract.

As such the finding doesn't prove any additional security risk beside those that comes with a multi-sig

For those reasons the finding is invalid

[kphed]

Thanks for following up with your thoughts @GalloDaSballo .

NOTE: Mistakenly made comment below because I thought this was referring to the BribeVault contract.

The sponsor is making it clear that the owner in this case is also the depositor of funds. This means that the multi-sig is self custodying the funds into the contract.

Just to clear up any miscommunication or misunderstandings, we've never stated that the owner is the depositor of funds - the funds are deposited by bribers. The owner/admin only whitelists contracts that have permission to call the BribeVault's deposit methods but those contracts do not custody funds beyond the deposit transactions (this is also only the case when a briber deposits a native token).

[GalloDaSballo]

Thank you for the clarification @kphed

If the bribers are not the same as the owner then the owner technically has the ability of withdrawing funds at any time, which puts the depositors under the risk of the owner rugging.

Typically a Vault Protocol (Yearn, Badger) would have a check for "protectedTokens", in this case BTRFLY and WETH to prevent taking that type of operation.

As it stands, the multisig can move the funds at any time, technically can frontrun the keeper and steal the funds.

Also notice that you said that there will be a keeper for performUpkeep but the modifier is onlyOwner which either means you'll have an EOA as the owner, or you may want to change the access control checker (or remove it as Chainlink docs would require you to).

With the information I have I'm inclined to revert back to medium severity.

While there's always the counter-argument that the multisig or governance will not rug, the only guarantee for it is the inability to rug by structuring the smart contract in a way that makes it impossible to move funds (e.g. add a check against moving BTRFLY and WETH, allow sweeping of other "random" tokens)

[kphed]

Sorry, disregard my last comment, I mistakenly read your comment as one directed towards BribeVault (which also has a method to withdraw tokens). You're correct, BTRFLY is minted by our protocol for ThecosomataETH. That said, we still don't consider the possibility of admin-rugging a real concern.

Typically a Vault Protocol (Yearn, Badger) would have a check for "protectedTokens", in this case BTRFLY and WETH to prevent taking that type of operation.

This is a potential idea, thanks, I'll share it with the team.

Also notice that you said that there will be a keeper for performUpkeep but the modifier is onlyOwner which either means you'll have an EOA as the owner, or you may want to change the access control checker (or remove it as Chainlink docs would require you to).

Tagging @drahrealm as he's implementing ThecosomataETH. Your comment about onlyOwner is good one though - it does appear to be a mistake or can be improved tremendously (e.g. use AccessControl and add a role limited to calling this and not the withdraw method).

[GalloDaSballo]

Would highly recommend limiting the withdrawal to specific tokens (ideally exclude important tokens), this would provide strong security guarantees against a rug.

Also limiting roles can help reduce trust, however, it wouldn't address the underlying issue that "someone" can move the funds.

With the information I have I believe Medium Severity to be appropriate, and believe the sponsor has set motion to minimize trust as well as add additional security guarantees