Platform: Code4rena
Start Date: 15/02/2022
Pot Size: $30,000 USDC
Total HM: 18
Participants: 35
Period: 3 days
Judge: GalloDaSballo
Total Solo HM: 8
Id: 87
League: ETH
Rank: 10/35
Findings: 3
Award: $970.15
🌟 Selected for report: 1
🚀 Solo Findings: 0
https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/BribeVault.sol#L330 https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/BribeVault.sol#L346
Both emergencyWithdrawERC20 and emergencyWithdraw allow any account with DEFAULT_ADMIN_ROLE to take all the bribes and send them to their own account.
This can be used as a rug vector.
Ideally the emergencyWithdraw functions would return all bribes to senders (can be done by tracking bribes via mappings), or send it a reimbursement contract.
It's important you disclose admin privileges to your users and depositors, and ideally you limit by changing the emergencyWithdraw functions to send the tokens back to the original depositors rather than having an admin take custody of the tokens
#0 - kphed
2022-02-17T00:08:28Z
#1 - GalloDaSballo
2022-03-05T16:36:03Z
Duplicate of #5
#2 - GalloDaSballo
2022-03-05T16:36:43Z
Notice: I'm forfetting all my winnings because am judging the contest
166.1732 USDC - $166.17
addLiquidity uses CURVE_POOL. calc_token_amount to determine what the expected LP tokens it should receive
It then calculates uint256 minAmount = expectedAmount - ((expectedAmount * slippage) / 1000);
However, because of the fact that transactions are atomic, any transaction that happened before this call could have changed the slippage, from what would be expected, to a less favourable setup.
Because the check happens right before addingLiquidity, it is effectively the same as having minAmount set to 1 (a slippage check, but an ineffective one)
The only reliable ways to perform a proper slippage check would be for either the caller to provide the actual minAmount or to use a priceFeed for it.
Alternatively, as a way to mitigate the finding, you could use Flashbots to run private transactions and avoid being front-run in the mempool
Send the minAmount from the caller, use a PriceFeed or use Private TX
#0 - drahrealm
2022-02-17T04:44:58Z
Idem with #35, we will proceed with doing calculating the min token amount off-chain, then specify it when calling performUpKeep.
Thanks for the finding.
#1 - GalloDaSballo
2022-03-05T17:48:20Z
Duplicate of #35
🌟 Selected for report: GalloDaSballo
Also found by: gzeon
759.8225 USDC - $759.82
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
Disclose the admin privilege in your docs Refactor the code to reduce it
#0 - kphed
2022-02-17T00:07:22Z
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.
#1 - GalloDaSballo
2022-02-25T17:57:48Z
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
#2 - kphed
2022-02-25T19:45:42Z
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).
#3 - GalloDaSballo
2022-02-25T20:11:21Z
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)
#4 - kphed
2022-02-26T03:10:11Z
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).
#5 - GalloDaSballo
2022-03-05T16:35:30Z
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