Platform: Code4rena
Start Date: 08/12/2021
Pot Size: $30,000 ETH
Total HM: 12
Participants: 26
Period: 3 days
Judge: leastwood
Total Solo HM: 9
Id: 65
League: ETH
Rank: 6/26
Findings: 4
Award: $1,862.53
🌟 Selected for report: 9
🚀 Solo Findings: 0
🌟 Selected for report: gzeon
Also found by: 0x0x0x, TomFrenchBlockchain
TomFrenchBlockchain
Publisher can make licenseFee arbitrarily large and then steal any funds remaining in the basket after 1 day.
On minting or burning basket tokens the handleFees function is called. This mints a number of basket tokens to the publisher and factory owner.
A key parameter in this calculation is the licenseFee which can be thought of as the percentage fee charged on the AUM over a year.
changeLicenseFee shows that while the publisher must wait a timelock period of 1 day to change the licenseFee, they can set it to an arbitrarily large value.
The attack is then as follows:
changeLicenseFee to prepare a change of the licenseFee to a huge value (e.g. 1e20 giving a 10000% yearly fee)This attack requires for other users in a basket to not leave during the first 1 day timelock period. While it's possible that all users will see this attack coming and avoid it, this is completely unrealistic especially with such a short timelock.
Enforce a maximum value for the rate at which the basket can charge fees.
Consider what powers those which receive fees have to influence their size.
#0 - frank-beard
2022-02-22T19:28:55Z
🌟 Selected for report: TomFrenchBlockchain
Also found by: WatchPug
690.8489 USDC - $690.85
TomFrenchBlockchain
All user funds in a basket being held hostage by the publisher
The Basket publisher can propose an auction in order to set new tokens and weights with a 1 day timelock.
As part of this call they can set the minIbRatio variable which determines what the maximum slippage on the auction is allowed to be. If it's set to the current IbRatio then the Basket accepts no slippage.
The publisher can choose to set minIbRatio = type(uint256).max which will prevent any auction bids from being successful, locking the basket in the auction state.
It's not possible to enter or exit the basket while an auction is going on, so any users who hold any funds in the basket are forced to take the only option to kill the auction available to them.
If a user makes a bond and then waits a day to then call Auction.bondBurn, it will reset the auction and allow users to withdraw but it requires 0.25% of the supply of the basket token to be burned.
One of the basket's users is then forced to give up some of their assets to secure the release of the remaining assets in the basket (for a 24hr period until the publisher starts a new auction).
This attack can be launched at any time with only 24 hours warning. This is a very short amount of time which near guarantees that if other users hold funds in the basket that not all of them will successfully withdraw in that time and so will have funds locked.
Again this is tricky to mitigate as there are legitimate scenarios where we would expect the ibRatio to increase. e.g. a basket containing WBTC being changed to contain USDC as each basket token should be worth much more USDC than it was in terms of WBTC.
To be frank the entire auction mechanism is a bit shaky as it doesn't account for changes in the values of the tokens over time.
#0 - frank-beard
2022-02-22T19:31:25Z
this is where community action and the timelock should mitigate attacks of these types. users should be able to hold publishers accountable for their rebalances, whether that is through a dao or other means. we acknowledge there is some level of trust required between the user and the publisher however this is also intentional, for the types of products this protocol is for.
#1 - 0xleastwood
2022-03-26T07:02:22Z
Because a timelock is used in this instance, exploiting this issue proves more difficult and requires that the publisher is malicious. As we are dealing with an abuse of privileges, I think this fits the criteria of a medium severity issue as the issue can only be exploited by a trusted account.
#2 - 0xleastwood
2022-03-27T02:08:47Z
Actually I'm not sure if I understand the exploit:
Basket.publishNewIndex(), setting new values and starting an auction. In that time, users are free to exit the protocol as they wish.Auction.bondForRebalance() in order to unlock the basket's underlying assets. But because Auction.settleAuction() will always revert, this user forfeits their bond to unlock the rest of their tokens.In this case, I see this as an abuse of the publisher's privileges and lack of oversight by the users. medium severity seems correct in this situation.
🌟 Selected for report: TomFrenchBlockchain
511.7399 USDC - $511.74
TomFrenchBlockchain
Needlessly restricting the pool of possible auction participants resulting in worse execution
The bonding mechanism doesn't seem to perform any meaningful purpose and leads to some significant downsides
Alternatively the auction could be set up to allow instant settlement to avoid these issues. The settlement transaction is potentially gas intensive with many token transfers so to avoid expensive reverting transactions in competitive auctions it may be desirable to split up transferring tokens to the auction contract to finish the auction and redeeming the tokens being claimed in return.
Medium risk due to the bonding mechanism causing the system to leak value through poor auction execution prices.
As above
#0 - frank-beard
2022-02-22T19:43:06Z
thank you for the feedback on the bonding mechanism. we are adding an option for settling an auction w/o bonding, however we believe the bonding mechanism is helpful and useful for some rebalancers
#1 - 0xleastwood
2022-03-27T03:03:16Z
I don't believe the issue highlighted qualifies as medium risk as the warden has not outlined how value is leaked from the protocol. I think this process can be improved by opening up the process to all participants.
🌟 Selected for report: 0x0x0x
Also found by: TomFrenchBlockchain, danb
11.0559 USDC - $11.06
TomFrenchBlockchain
Gas costs
There's a check after the basket mints some token that the total supply doesn't exceed the max supply. https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L103
However immediately before the mint we test whether totalsupply + amount > maxSupply which would fail in any situation where the post-mint check would fail.
Remove post-mint check on totalsupply.
#0 - 0xleastwood
2022-03-27T10:54:28Z
Duplicate of #142
🌟 Selected for report: WatchPug
Also found by: TomFrenchBlockchain
18.4266 USDC - $18.43
TomFrenchBlockchain
Gas costs
On these lines we query the factory.ownerSplit() twice, spending gas for duplicated calculations
Cache the result of a single call of factory.ownerSplit() instead
#0 - 0xleastwood
2022-03-27T11:01:30Z
Duplicate of #122
🌟 Selected for report: GiveMeTestEther
Also found by: Jujic, TomFrenchBlockchain, WatchPug, gzeon, kenzo, sirhashalot, ye0lde
TomFrenchBlockchain
Gas costs of deployment
In the factory constructor we always set the ownerSplit to zero explicitly
ownerSplit will already be zero implicitly so by setting it this way we're just wasting gas.
Remove the highlighted line or allow passing in a nonzero value.
#0 - 0xleastwood
2022-03-27T11:09:30Z
Duplicate of #45
🌟 Selected for report: WatchPug
Also found by: GiveMeTestEther, TomFrenchBlockchain, danb, kenzo
TomFrenchBlockchain
Gas costs
Here we perform an O(n^2) check for duplicate tokens
for (uint i = 0; i < length; i++) { require(_tokens[i] != address(0)); require(_weights[i] > 0); for (uint256 x = 0; x < tokenList.length; x++) { require(_tokens[i] != tokenList[x]); } tokenList[i] = _tokens[i]; }
can be replaced with
require(_tokens[0] != address(0)); require(_weights[0] > 0); for (uint i = 1; i < length; i++) { require(_weights[i] > 0); require(tokens[i] > tokens[i-1]); }
which is O(n).
As above
#0 - 0xleastwood
2022-03-27T11:08:05Z
I think this is a valid suggestion,
#1 - 0xleastwood
2022-03-27T22:46:41Z
Duplicate of #118
🌟 Selected for report: TomFrenchBlockchain
TomFrenchBlockchain
Extra gas costs on deploying new Baskets
In the Factory.createBasket function we transfer all of the user's tokens to the factory and then call the Basket.mintTo function which transfers all of these tokens again from the factory to the basket.
As the Factory can be trusted to provide the correct number of tokens to the Basket, it could instead directly transfer all the tokens to the basket's address as simple transfers. Basket.initialize would then have to be modified to mint BASE tokens to the user as a one-off to ensure proper accounting.
// Old newBasket.initialize(bProposal, newAuction); for (uint256 i = 0; i < bProposal.weights.length; i++) { IERC20 token = IERC20(bProposal.tokens[i]); token.safeTransferFrom(msg.sender, address(this), bProposal.weights[i]); token.safeApprove(address(newBasket), bProposal.weights[i]); } newBasket.mintTo(BASE, msg.sender); // New newBasket.initialize(bProposal, newAuction, msg.sender); // mints BASE to msg.sender for (uint256 i = 0; i < bProposal.weights.length; i++) { IERC20 token = IERC20(bProposal.tokens[i]); token.safeTransferFrom(msg.sender, address(newBasket), bProposal.weights[i]); }
As above.
🌟 Selected for report: TomFrenchBlockchain
TomFrenchBlockchain
Extra gas costs on all auction operations
When starting an auction we set auctionOngoing = true and auctionStart = block.number. auctionStart is then unmodified until the next auction is started while auctionOngoing is checked to see if an auction is in progress.
If Auction.killAuction were to reset auctionStart = 0, we could remove auctionOngoing and test for whether an auction is ongoing by checking that auctionStart is nonzero, saving an SLOAD.
As above.
🌟 Selected for report: TomFrenchBlockchain
TomFrenchBlockchain
Extra gas costs on auction bonding and settlement
Similarly to finding "Auction.auctionOngoing variable is unnecessary" bondTimestamp, bondBlock and auctionBonder are all good candidates for replacing use of hasBonded by checking against the sentinel value of zero.
As described above
🌟 Selected for report: TomFrenchBlockchain
Also found by: kenzo
TomFrenchBlockchain
Gas costs
On this line we query the baskets total supply pendingWeights.length times spending gas for duplicated calculations
Cache the result of a single call of basketAsERC20.totalSupply() instead
🌟 Selected for report: TomFrenchBlockchain
TomFrenchBlockchain
Hundreds of extra gas spent on every mint/burn
Minting and burning the Basket tokens emits Minted and Burned events. These aren't needed as mints and burns can be tracked using the Transfer events to and from the zero address emitted within the _mint function.
Rely on Transfer events rather than custom Minted and Burned events.
#0 - 0xleastwood
2022-03-27T11:02:28Z
I can see reasons why these events are emitted but the sponsor has not disputed this so leaving it open.
🌟 Selected for report: TomFrenchBlockchain
Also found by: WatchPug
TomFrenchBlockchain
Gas costs
pendingPublisher.publisher != address(0) && pendingPublisher.publisher == newPublisher
can safely be replaced with
pendingPublisher.publisher == newPublisher
As there's no way to pass in newPublisher == address(0) as enforced by the previous require statement.
As above
#0 - 0xleastwood
2022-03-27T11:06:31Z
The initial publisher must be a non-zero address so it isn't possible to set the publisher to the zero address already as newPublisher != address(0) is checked.
🌟 Selected for report: TomFrenchBlockchain
TomFrenchBlockchain
gas costs
In the factory auctionImpl and basketImpl can be made immutable to save gas costs on basket deployment
Make these two variables immutable.