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: 1/30
Findings: 6
Award: $7,266.85
🌟 Selected for report: 9
🚀 Solo Findings: 4
🌟 Selected for report: WatchPug
3339.7729 USDC - $3,339.77
WatchPug
Given the auctionBurn() function will _burn() the auction bond without updating the ibRatio. Once the bond of a failed auction is burned, the proportional underlying tokens won't be able to be withdrawn, in other words, being frozen in the contract.
With the configuration of:
basket.ibRatio = 1e18 factory.bondPercentDiv = 400 basket.totalSupply = 400 basket.tokens = [BTC, ETH] basket.weights = [1, 1]
auctionBurn();basket.ibRatio remains to be 1e18; basket.totalSupply = 399.
Burn 1 BASKET TOKEN will only get back 1 BTC and 1 ETH, which means, there are 1 BTC and 1 ETH frozen in the contract.
Change to:
function auctionBurn(uint256 amount) onlyAuction external override { handleFees(); uint256 startSupply = totalSupply(); _burn(msg.sender, amount); uint256 newIbRatio = ibRatio * startSupply / (startSupply - amount); ibRatio = newIbRatio; emit NewIBRatio(newIbRatio); emit Burned(msg.sender, amount); }
#0 - GalloDaSballo
2021-12-19T00:10:38Z
The warden has identified a way for funds to be stuck without a way to recoup them, this is because ibRatio is not updated, while totalSupply is.
Because this is a specific accounting error, which is effectively a bug in the logic of the protocol, and funds can be irrevocably lost, this is a high severity finding
🌟 Selected for report: WatchPug
1001.9319 USDC - $1,001.93
WatchPug
function handleFees() private { if (lastFee == 0) { lastFee = block.timestamp; } else { uint256 startSupply = totalSupply(); uint256 timeDiff = (block.timestamp - lastFee); uint256 feePct = timeDiff * licenseFee / ONE_YEAR; uint256 fee = startSupply * feePct / (BASE - feePct); _mint(publisher, fee * (BASE - factory.ownerSplit()) / BASE); _mint(Ownable(address(factory)).owner(), fee * factory.ownerSplit() / BASE); lastFee = block.timestamp; uint256 newIbRatio = ibRatio * startSupply / totalSupply(); ibRatio = newIbRatio; emit NewIBRatio(ibRatio); } }
timeDiff * licenseFee can be greater than ONE_YEAR when timeDiff and/or licenseFee is large enough, which makes feePct to be greater than BASE so that BASE - feePct will revert on underflow.
Minting and burning of the basket token are being disrupted until the publisher update the licenseFee.
licenseFee of 1e19 or 1000% per year and mint 1 basket token;mint and burn reverts at handleFees().Limit the max value of feePct.
#0 - GalloDaSballo
2021-12-19T00:18:43Z
The finding is valid, there are conditions that would cause feePct to be greater than BASE
The conditions to trigger this seem to be:
Because this can happen under specific conditions, I will grade this finding as medium severity:
I would highly recommend the sponsor to consider the possibility of capping the licenseFee to make it easier to predict cases in which the operation can revert
🌟 Selected for report: WatchPug
1001.9319 USDC - $1,001.93
WatchPug
The newRatio that determines tokensNeeded to settle the auction is calculated based on auctionMultiplier, bondTimestamp - auctionStart and auctionDecrement.
uint256 a = factory.auctionMultiplier() * basket.ibRatio(); uint256 b = (bondTimestamp - auctionStart) * BASE / factory.auctionDecrement(); uint256 newRatio = a - b;
However, if an auction is bonded late (bondTimestamp - auctionStart is a large number), and/or the auctionMultiplier is small enough, and/or the auctionDecrement is small enough, that makes b to be greater than a, so that uint256 newRatio = a - b; will revert on underflow.
This might seem to be an edge case issue, but considering that a rebalance auction of a bag of shitcoin to high-value tokens might just end up being bonded at the last minute, with a newRatio near zero. When we take the time between the bonder submits the transaction and it got packed into a block, it's quite possible that the final bondTimestamp gets large enough to revet a - b.
An auction successfully bonded by a regular user won't be able to be settled, and the user will lose the bond.
With the configuration of:
basket.ibRatio = 1e18 factory.auctionDecrement = 5760 (Blocks per day) factory.auctionMultiplier = 2
bondForRebalance() and it will succeed;settleAuction() will always revert.Calculate and require newRatio > 0 in bondForRebalance(), or limit the max value of decrement and make sure newRatio always > 0 in settleAuction().
#0 - GalloDaSballo
2021-12-19T00:22:29Z
Finding is valid, there are cases that can cause a - b to revert
The finding is reliant on external condition (multiplier being low and / or bonding late) as such I believe this to be a medium severity finding.
Note for the sponsor: The cause for reverts should get clearer with time (as people use the protocol), you definitely want to model these revert behaviour to avoid edge cases that will make funds stuck
🌟 Selected for report: WatchPug
1001.9319 USDC - $1,001.93
WatchPug
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L143
function withdrawBounty(uint256[] memory bountyIds) internal { // withdraw bounties for (uint256 i = 0; i < bountyIds.length; i++) { Bounty memory bounty = _bounties[bountyIds[i]]; require(bounty.active); IERC20(bounty.token).transfer(msg.sender, bounty.amount); bounty.active = false; emit BountyClaimed(msg.sender, bounty.token, bounty.amount, bountyIds[i]); } }
In the withdrawBounty function, bounty.active should be set to false when the bounty is claimed.
However, since bounty is stored in memory, the state update will not succeed.
An auction successfully bonded by a regular user won't be able to be settled if they passed seemly active bountyIds, and the bonder will lose the bond.
settleAuction() with the bountyIds of the 2 seemly active bounties always reverts.Change to:
Bounty storage bounty = _bounties[bountyIds[i]];
#0 - frank-beard
2021-10-06T16:29:04Z
#1 - GalloDaSballo
2021-12-19T15:48:08Z
Finding is valid, because the warden didn't provide a POC of how to steal user funds, the finding is of medium severity
🌟 Selected for report: WatchPug
333.9773 USDC - $333.98
WatchPug
As defined in the ERC20 Specification, the approve function returns a bool that signals the success of the call. However, in Basket.sol#approveUnderlying() the value returned from calls to approve is ignored.
To handle calls to approve safely, consider using the safeApprove function in OpenZeppelin’s SafeERC20 contract for all approvals.
#0 - GalloDaSballo
2021-11-30T23:12:14Z
Agree with finding, raising severity to 1 as in some cases the approval can fail and it would be best to revert
🌟 Selected for report: WatchPug
WatchPug
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L102
Anyone can call addBounty() to add a bounty with any token. If we assume that the frontend will always pass all the bountyIDs of active bounties to settleAuction(), then a malicious user can disrupt settleAuction() by addBounty with a fake token that always reverts when calling transfer().
Auction bonder wont be able to settleAuction(). The malicious user and all other holders of the basket token can belifits from the burn of the auction bond.
transfer();addBounty with the fake token and any amount;settleAuction with bountyIDs including the fake token bounty will always fail.Whitelist bounty tokens in smart contract or frontend.
#0 - GalloDaSballo
2021-12-19T22:15:20Z
By adding a malicious token as bounty, we can make the settleAuction transaction revert.
Simple mitigation is to not claim that bounty.
Generally speaking this can be viewed as medium severity because it's dependent on an external condition. But personally I think mitigation and likelihood are so low that I'll downgrade to low.
Will mark as low, because all it takes to avoid this type of griefing is to remove the bounty from the list, the fact that the frontend may not has no relevance for this contest
150.2898 USDC - $150.29
WatchPug
The functions should first check if the passed arguments are valid first. The checks-effects-interactions pattern should be implemented throughout the code.
Scenario: If an incorrect auctionDecrement was set, all ongoing auctions could potentially be bonded and settled at an unfair price, case fund loss to all basket token holders.
The Factory contract allows the owner to change various critical parameters of the protocol. As such, contract ownership plays a critical role in the protocol.
Several critical operations are done in one function call. This schema is error-prone and can lead to irrevocable mistakes.
Scenario: If an incorrect address was used for transferOwnership, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various critical parameters.
Consider adding checks for incorrect arguments, and adding timelocks for the changing of critical addresses and parameters. Or consider using a timelock contract as the owner of the Factory contract.
#0 - GalloDaSballo
2021-12-20T00:51:16Z
Grouping up the findigns for lack of checks and two-step for setters, am going to set all of these to low severity
4.3799 USDC - $4.38
WatchPug
Functions that are never called throughout the contract should be marked as external visibility instead of public visibility to avoid the unnecessary copying of data to memory.
This will effectively result in Gas Optimization.
Therefore, the following function must be marked as external:
Factory.sol
Basket.sol
Auction.sol
The above-mentioned functions should be assigned external visibility, and the parameter storage location should be set as calldata rather than memory.
#0 - GalloDaSballo
2021-11-29T13:57:48Z
Duplicate of #240
15.5766 USDC - $15.58
WatchPug
Every call to an external contract costs a decent amount of gas. For optimization of gas usage, external call results should be cached if they are being used for more than one time.
For example:
_mint(publisher, fee * (BASE - factory.ownerSplit()) / BASE); _mint(Ownable(address(factory)).owner(), fee * factory.ownerSplit() / BASE);
Can be changed to:
uint256 ownerSplit = factory.ownerSplit(); _mint(publisher, fee * (BASE - ownerSplit) / BASE); _mint(Ownable(address(factory)).owner(), fee * ownerSplit / BASE);
#0 - GalloDaSballo
2021-11-29T14:02:54Z
Duplicate of #43
15.5766 USDC - $15.58
WatchPug
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Several instances include:
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Factory.sol#L103
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L81
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L85
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L96
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L142
For example:
function pushUnderlying(uint256 amount, address to) private { for (uint256 i = 0; i < weights.length; i++) { uint256 tokenAmount = amount * weights[i] * ibRatio / BASE / BASE; IERC20(tokens[i]).safeTransfer(to, tokenAmount); } }
Can be changed to:
function pushUnderlying(uint256 amount, address to) private { uint256 length = weights.length; for (uint256 i = 0; i < length; i++) { uint256 tokenAmount = amount * weights[i] * ibRatio / BASE / BASE; IERC20(tokens[i]).safeTransfer(to, tokenAmount); } }
#0 - GalloDaSballo
2021-11-29T14:03:59Z
Duplicate of #230
25.9609 USDC - $25.96
WatchPug
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
For example:
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L135
uint256 id = _bounties.length - 1;
Can be changed to:
uint256 id; unchecked { id = _bounties.length - 1; }
#0 - GalloDaSballo
2021-11-29T14:02:14Z
When using Solidity with version >= 0.8.X you can use unchecked to save about 30 / 40 gas, at the cost of readability
15.5766 USDC - $15.58
WatchPug
timeDiff in Basket.sol#handleFees() can be 0 when there are multiple mint and burn transactions in the same block. Check if timeDiff > 0 can save gas from unnecessary code execution like mint with amount 0.
Change to:
function handleFees() private { if (lastFee == 0) { lastFee = block.timestamp; } else { uint256 timeDiff = (block.timestamp - lastFee); if (timeDiff == 0) return; uint256 startSupply = totalSupply(); uint256 feePct = timeDiff * licenseFee / ONE_YEAR; uint256 fee = startSupply * feePct / (BASE - feePct); _mint(publisher, fee * (BASE - factory.ownerSplit()) / BASE); _mint(Ownable(address(factory)).owner(), fee * factory.ownerSplit() / BASE); lastFee = block.timestamp; uint256 newIbRatio = ibRatio * startSupply / totalSupply(); ibRatio = newIbRatio; emit NewIBRatio(ibRatio); } }
#0 - GalloDaSballo
2021-11-29T13:57:21Z
Duplicate of #53
25.9609 USDC - $25.96
WatchPug
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L47-L52
Auction.sol#initialize() is using the factory_ parameter as the value of factory, while Basket.sol#initialize() uses msg.sender.
https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L39
Consider changing to msg.sender and remove the factory_ parameter for the purpose of consistency and gas saving.
#0 - GalloDaSballo
2021-11-29T14:01:26Z
Finding is valid, savings are minor