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: 15/30
Findings: 3
Award: $976.69
🌟 Selected for report: 6
🚀 Solo Findings: 0
270.5216 USDC - $270.52
pauliax
The same bounty can be claimed multiple times as it remains active. 'bounty' points to the memory variable, so its state is not persisted: Bounty memory bounty = _bounties[bountyIds[i]]; require(bounty.active); IERC20(bounty.token).transfer(msg.sender, bounty.amount); bounty.active = false;
A simple solution would be to use a storage pointer instead: Bounty storage bounty = _bounties[bountyIds[i]];
#0 - frank-beard
2021-09-28T21:31:14Z
#1 - GalloDaSballo
2021-12-19T15:47:20Z
Duplicate of #168
#2 - GalloDaSballo
2021-12-27T00:44:03Z
I ended up downgrading this finding as well as marking as duplicate because the warden didn't specify any attack vector via the vulnerability.
43.8245 USDC - $43.82
pauliax
Consider using ReentrancyGuard to protect functions that have external calls and do not follow Checks Effects Interactions pattern. An example of a function that needs to prevent re-entrancy is settleAuction as it calls withdrawBounty before updating the state and because anyone can add new bounties with no restrictions, it may contain tokens with callbacks on transfer (e.g. erc777) which may call this function again and again.
#0 - frank-beard
2021-10-19T17:20:32Z
#1 - GalloDaSballo
2021-12-01T22:34:45Z
Duplicate of #270
🌟 Selected for report: pauliax
333.9773 USDC - $333.98
pauliax
Consider checking the actual amount transferred, e.g. in function addBounty as otherwise, you may have trouble with non-standard tokens, e.g. deflationary with a fee on transfer. The actual amount transferred is balanceOf the token before and after.
uint256 before = IERC20(token).balanceOf(address(this)); token.safeTransferFrom(msg.sender, address(this), amount); uint256 after = IERC20(token).balanceOf(address(this)); amount = after - before;
#0 - frank-beard
2021-10-19T17:20:59Z
#1 - GalloDaSballo
2021-12-02T00:48:53Z
This finding comments on the correctness of trusting input vs actually checking what the changed balance is. There is no clear "attack" nor way to brick or grief the protocol. Agree with finding and severity
🌟 Selected for report: pauliax
57.691 USDC - $57.69
pauliax
You can get rid of hasBonded state variable if you treat bondTimestamp with a value of 0 as not bonded. This would save some gas.
E.g.: replace require(!hasBonded); with require(bondTimestamp == 0); and hasBonded = false; with bondTimestamp = 0;
#0 - GalloDaSballo
2021-11-26T01:18:18Z
Agree that you can remove the boolean hasBonded by giving a special meaning to the value 0 for bondTimestamp
🌟 Selected for report: pauliax
57.691 USDC - $57.69
pauliax
It is unclear why you need this new local variable called newIbRatio if you instantly update and use the storage variable afterwards: uint256 newIbRatio = ibRatio * startSupply / totalSupply(); ibRatio = newIbRatio;
ibRatio = ibRatio * startSupply / totalSupply();
#0 - GalloDaSballo
2021-11-26T01:19:15Z
Agree with the finding, note that would save only 6 gas (3 for storing in memory, 3 for reading for the copy to storage)
🌟 Selected for report: pauliax
57.691 USDC - $57.69
pauliax
This can be refactored to improve precision and gas usage: _mint(publisher, fee * (BASE - factory.ownerSplit()) / BASE); _mint(Ownable(address(factory)).owner(), fee * factory.ownerSplit() / BASE);
Proposed solution: uint256 factoryOwnerFee = fee * factory.ownerSplit() / BASE; uint256 publisherFee = fee - factoryOwnerFee; _mint(Ownable(address(factory)).owner(), factoryOwnerFee); _mint(publisher, publisherFee); This will result in fewer math operations and better precision cuz multiplication and division are replaced with subtraction.
#0 - GalloDaSballo
2021-11-26T01:23:04Z
Clever little optimization
🌟 Selected for report: leastwood
Also found by: cmichel, csanuragjain, itsmeSTYJ, joeysantoro, kenzo, pauliax
4.3799 USDC - $4.38
pauliax
I assume function validateWeights is very gas-consuming as it iterates against all the tokens again and again and re-assigns dynamic arrays multiple times. I think a good improvement that you may consider would be to ask the _tokens array to be provided in descending order, then a validation would be much cheaper.
An example solution:
// tokens must be in a descending order function validateWeights(address[] memory _tokens, uint256[] memory _weights) public pure { uint256 length = _tokens.length; require(length == _weights.length); uint lastIndex = length - 1; // check uniqueness of tokens and not token(0) for (uint i = 0; i < length; i++) { require(_tokens[i] != address(0)); require(_weights[i] > 0); if (i != lastIndex) { require(_tokens[i] > _tokens[i + 1], "descending"); } } }
#0 - frank-beard
2021-10-19T17:56:57Z
#1 - GalloDaSballo
2021-11-26T16:43:09Z
Duplicate of #160
4.3799 USDC - $4.38
pauliax
Functions like publishNewIndex deleteNewIndex, changeLicenseFee, changePublisher, etc can be 'external' not 'public' to save some gas as they are never called from the same contract.
#0 - frank-beard
2021-10-19T17:57:10Z
#1 - GalloDaSballo
2021-11-26T01:26:22Z
Duplicate of #240
🌟 Selected for report: pauliax
57.691 USDC - $57.69
pauliax
BLOCK_DECREMENT state variable in Auction is not used anywhere.
Consider removing unused variables.
#0 - GalloDaSballo
2021-11-26T01:26:57Z
Agree with the finding
🌟 Selected for report: pauliax
57.691 USDC - $57.69
pauliax
This double division by BASE can be eliminated to improve precision and reduce gas costs: uint256 tokensNeeded = basketAsERC20.totalSupply() * pendingWeights[i] * newRatio / BASE / BASE;
if you introduce a constant variable, e.g.: uint256 private constant BASE_2X = BASE * 2; uint256 tokensNeeded = basketAsERC20.totalSupply() * pendingWeights[i] * newRatio / BASE_2X;
#0 - GalloDaSballo
2021-11-26T16:35:47Z
Agree with the gas cost reduction as the value of BASE_2X will be inlined in the bytecode Can't confirm on the precision as you're still dividing by the same value
15.5766 USDC - $15.58
pauliax
You should cache the results of external calls when done in the same function multiple times and the result is not supposed to change between these calls, e.g. function handleFees calls factory.ownerSplit() twice. It would be more gas efficient to store the results in a local variable and re-use where necessary.
#0 - GalloDaSballo
2021-11-26T16:36:47Z
Agree with the finding but am not happy with the submission making an example without link Would like either a link, the submission being for only one finding, or the submission being a list of all instances
#1 - GalloDaSballo
2021-12-27T00:28:36Z
Duplicate of #43
15.5766 USDC - $15.58
pauliax
There are loops that iterate over storage arrays. Accessing the length of the array in every iteration is more expensive, so you should cache it and use this local variable when iterating over the storage array.
Example: uint bProposalWightsLength = bProposal.weights.length; for (uint256 i = 0; i < bProposalWightsLength; i++)
#0 - GalloDaSballo
2021-11-26T16:37:57Z
Agree with the finding, as a general rule you always want to cache storage values in memory if they are read more than once. Additionally, caching .length saves an extra math operation as the length is stored in a specific storage location that has to be re-calculated every loop
#1 - GalloDaSballo
2021-11-26T17:39:15Z
Duplicate of #230