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: 8/30
Findings: 4
Award: $1,900.29
🌟 Selected for report: 4
🚀 Solo Findings: 1
131.4735 USDC - $131.47
leastwood
The contracts Basket.sol and Auction.sol do not check the return values of several external calls to ERC20 tokens. Some ERC20 tokens may return false for a transfer() call, however, Auction.sol will treat the call as successful.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L101 https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L146 https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L226
Manual code review
Consider using Openzeppelin's SafeERC20 library to replace all transfer() and approve() calls with safeTransfer() and safeApprove() respectively. This ensures the return values of these external calls are correctly checked.
#0 - frank-beard
2021-10-19T17:03:55Z
#1 - GalloDaSballo
2021-12-12T18:11:15Z
Duplicate of #196
🌟 Selected for report: leastwood
1001.9319 USDC - $1,001.93
leastwood
The onlyOwner role is able to make changes to the protocol with an immediate affect, while other changes made in Basket.sol and Auction.sol incur a one day timelock. As a result, an onlyOwner role may unintentionally frontrun a settleAuction() transaction by making changes to auctionDecrement and auctionMultiplier, potentially causing the auction bonder to over compensate during a rebalance. Additionally, there is no way for an auction bonder to recover their tokens in the event this does happen.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Factory.sol#L39-L59 https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L89-L99
Manual code review
Consider adding a timelock delay to all functions affecting protocol execution. Alternatively, bondForRebalance() can set state variables for any external calls made to Factory.sol (i.e. factory.auctionMultiplier() and factory.auctionDecrement()), ensuring that settleAuction() is called according to these expected results.
#0 - frank-beard
2021-09-30T18:40:57Z
it is assumed the owner is trustworthy in this version of the protocol, however we will add mitigations and further decentralization in future updates
#1 - GalloDaSballo
2021-12-18T13:54:41Z
Agree with the finding, users are taking "owner privileges" risks while interacting with the protocol. The warden has identified a specific grief / DOS that the owner can cause
🌟 Selected for report: leastwood
333.9773 USDC - $333.98
leastwood
Several functions in Factory.sol do not emit events for state changes performed, making it difficult to log changes in frontend applications. The inputs of these functions are also not validated. These should be validated to ensure the new values are properly bounded and do not break functionality.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Factory.sol#L39-L59
Manual code review
Consider logging events for any state changes made. Additionally, ensure the new values fit within expected ranges.
#0 - GalloDaSballo
2021-12-02T01:11:20Z
I agree with the lack of validation being a low severity finding The risk is posed in the ability of the owner to set values that can potentially grief depositors or brick the protocol
I disagree with events, you can filter them via the eth_rpc endpoint and find the function calls to the contract
🌟 Selected for report: leastwood
333.9773 USDC - $333.98
leastwood
Factory.sol inherits Openzeppelin's Ownable contract which is used to restrict certain functions to a specific owner account. By default, the owner account is set to the deployer of Factory.sol. However, the owner can transfer ownership of this contract by calling transferOwnership() with the newOwner as input. If the current owner of the contract mistakenly transfers ownership to an account that is not actively controlled, the ownership of Factory.sol will be lost.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol
Manual code review.
Consider implementing a proper transfer ownership pattern whereby the current owner nominates a new owner. This new owner then has to claim ownership for the change to be applied. This can be done by overriding the current Ownable implementation of transferOwnership() and adding a claimOwnership() function alongside it.
#0 - frank-beard
2021-10-19T17:03:15Z
not an exploit but a good point to bring up. it is assumed the owner is trusted.
#1 - GalloDaSballo
2021-12-12T18:02:07Z
Similarly to #185 I agree with the finding but will downgrade to Low
90.1739 USDC - $90.17
leastwood
The addBounty() and withdrawBounty() functions are used to further incentivize basket rebalances. Bounty tokens are not vetted and therefore any token can be added to the Auction.sol contract to be later distributed to the auction bonder. Expecting the auction bonder to check all bounty token addresses before calling settleAuction() is optimistic and can cause rebalancing delays. IERC20(bounty.token).transfer(msg.sender, bounty.amount) makes an external call to an untrusted address. If the target address' transfer() function reverts for any reason, the entire settleAuction() call is reverted. The list of _bounties can be diluted by malicious users seeking to delay basket rebalances.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L126-L138 https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L140-L152 https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L102
Manual code review
Consider restricting bounty tokens to a list of trusted tokens, potentially the same list of tokens used within Basket.sol. An alternative approach would be to use try and catch statements when iterating through bountyIds and calling transfer().
#0 - frank-beard
2021-10-19T17:09:21Z
#1 - GalloDaSballo
2021-12-20T00:55:43Z
Duplicate of #55
4.3799 USDC - $4.38
leastwood
There are a number of bools in Auction.sol which can be combined to fit into a single slot. This saves gas on each deployment of the Auction.sol contract.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L16-L21
Manual code review
Consider implementing proper slot packing in all contracts.
#0 - GalloDaSballo
2021-11-26T16:38:56Z
Duplicate of #109
🌟 Selected for report: leastwood
Also found by: cmichel, csanuragjain, itsmeSTYJ, joeysantoro, kenzo, pauliax
4.3799 USDC - $4.38
leastwood
The validateWeights() function can be better optimised by using a hashmap to measure token uniqueness. Currently, the function utilises an O(n^2) solution. By first iterating through each hashmap index for _tokens, any previously set tokens can be first cleared . This improves the current solution to O(n).
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L53-L70
Manual code review
Consider using a hashmap to measure token uniqueness. However, this hashmap needs to first be cleared out before using it each time in validateWeights().
#0 - GalloDaSballo
2021-11-26T16:42:39Z
Agree with the finding, and prefer the suggestion which keeps it fully trustless