Platform: Code4rena
Start Date: 16/09/2021
Pot Size: $50,000 USDC
Total HM: 26
Participants: 30
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 17
Id: 36
League: ETH
Rank: 11/30
Findings: 4
Award: $1,515.58
🌟 Selected for report: 2
🚀 Solo Findings: 1
🌟 Selected for report: 0xsanson
1001.9319 USDC - $1,001.93
0xsanson
The owner of Factory contract can modify the values of auctionMultiplier and auctionDecrement at any time.
During an auction, these values are used to calculate newRatio and thereby tokensNeeded: specifically, it's easy to set the factory parameters so that tokensNeeded = 0 (or close to zero) for every token.
This way the owner can participate at an auction, change the parameters, and get the underlying tokens from a Basket without transferring any pending tokens.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L89-L99
editor
Consider adding a Timelock to these Factory functions. Otherwise a way to not modify them if an auction is ongoing (maybe Auction saves the values it reads when startAuction is called).
#0 - frank-beard
2021-09-28T20:47:30Z
the owner for this is intended to be a dao that acts in support of the protocol, however this is a good point to the centralization concerns for the protocol, we will most likely manage this by adding a timelock to these function
#1 - GalloDaSballo
2021-12-19T00:01:18Z
Agree with the finding from the warden, highly recommend the sponsor to add these possibilities in their docs to make it easy for users to understand them.
That said, a possible remediation would also be to add checks in the setters, to avoid for these specific edge cases.
Because the exploit is dependent on an external condition (setting to a specific value by governance), the finding is of medium severity
131.4735 USDC - $131.47
0xsanson
In Basket.sol, approveUnderlying is used to approve tokens to be spent by the Auction.
The current implementation uses a simple approve function, instead of the safer safeApprove. Also it's recommended to have an approve to zero first, since the allowance could be already positive and some tokens (e.g. USDT) wouldn't work in this case.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L226
editor
Consider writing
IERC20(tokens[i]).safeApprove(spender, 0); IERC20(tokens[i]).safeApprove(spender, type(uint256).max);
#0 - GalloDaSballo
2021-12-19T15:55:07Z
Duplicate of #35
43.8245 USDC - $43.82
0xsanson
In Auction.settleAuction, the auctionBonder can withdraw some bounty tokens from the contract.
Since these can be any tokens, the bonder can donate his special """token""" first, then call settleAuction: at the line withdrawBounty(bountyIDs) the execution is passed to their custom contract.
A re-entrancy here is also possible since auctionOngoing and hasBonded are modified only at the end of the function.
I don't see how this can cause any issue at the moment, but if in the future this contract and Basket.sol are upgraded it's better to play safe and mitigate possible issues now.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L102
editor
Consider moving auctionOngoing = false and hasBonded = false before any uncontrolled external contract call.
#0 - frank-beard
2021-10-19T17:14:59Z
#1 - GalloDaSballo
2021-12-20T01:04:39Z
Duplicate of #270
🌟 Selected for report: 0xsanson
333.9773 USDC - $333.98
0xsanson
In Basket.sol, handleFees computes the following:
uint256 newIbRatio = ibRatio * startSupply / totalSupply().
In the case that totalSupply() = 0 (every holder burned their basket), the function reverts since there's a 0/0. This issue won't let new people mint, since handleFees is called before any minting.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L124
editor
Consider adding a check before the division.
if (startSupply == 0) { return; }
#0 - GalloDaSballo
2021-12-12T16:26:15Z
I agree with the finding
I think the warden may have missed a bigger issue (you seem to not be able to mint as mint calls handleFees)
Given the info received the finding is valid, and the severity is valid as well
Highly recommend the sponsor checks the revert for minting as well
4.3799 USDC - $4.38
0xsanson
The function Factory.proposeBasketLicense isn't used inside Factory contract, so it can be set to external to save gas.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Factory.sol#L65
#0 - frank-beard
2021-10-19T17:52:04Z
#1 - GalloDaSballo
2021-11-29T14:12:54Z
Duplicate of #240