Platform: Code4rena
Start Date: 25/11/2021
Pot Size: $80,000 USDC
Total HM: 35
Participants: 32
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 27
Id: 59
League: ETH
Rank: 10/32
Findings: 3
Award: $2,370.41
🌟 Selected for report: 10
🚀 Solo Findings: 0
🌟 Selected for report: thank_you
Also found by: 0x0x0x, Koustre, Meta0xNull, WatchPug, cmichel, defsec, harleythedog, hyh, leastwood, pauliax, pmerkleplant, tabish, xYrYuYx
pauliax
functions buyMalt and sellMalt, and removeLiquidity have no slippage protection and addLiquidity hardcodes it to 5%:
0, // amountOutMin
Mempool snipers can profit from that by monitoring the chain and sandwiching these functions. Now it is left for the caller to check if he is satisfied with the received amount. For example, function _unbondAndBreak is happy with any amount:
(uint256 amountMalt, uint256 amountReward) = dexHandler.removeLiquidity(); malt.safeTransfer(msg.sender, amountMalt); rewardToken.safeTransfer(msg.sender, amountReward);
See a similar finding in the previous contest: https://github.com/code-423n4/2021-09-bvecvx-findings/issues/34
While it may be impossible in certain cases to predict or specify it, you should consider reviewing all the cases and hardening the slippage protection where possible.
#0 - 0xScotch
2021-12-10T00:21:22Z
#219
#1 - GalloDaSballo
2022-01-24T00:30:04Z
Duplicate of #219
165.5635 USDC - $165.56
pauliax
I think this if check is incorrect, because in theory maxAmount parameter can be greater than totalMaltBalance:
if (rewards <= deployedCapital && maxAmount != totalMaltBalance) { // If all malt is spent we want to reset deployed capital deployedCapital = deployedCapital - rewards; } else { deployedCapital = 0; }
If my assumption is correct, the check should use balance, not maxAmount: ``solidity balance != totalMaltBalance
Another possible solution: ``solidity maxAmount <= totalMaltBalance
However, I think the best approach would be to eliminate 'balance' altogether:
uint256 totalMaltBalance = malt.balanceOf(address(this)); if (totalMaltBalance == 0) { return 0; } (uint256 basis,) = costBasis(); if (maxAmount > totalMaltBalance) { maxAmount = totalMaltBalance; } malt.safeTransfer(address(dexHandler), maxAmount); uint256 rewards = dexHandler.sellMalt(); if (rewards <= deployedCapital && maxAmount < totalMaltBalance) { // If all malt is spent we want to reset deployed capital deployedCapital = deployedCapital - rewards; } else { deployedCapital = 0; }
#0 - GalloDaSballo
2022-01-24T00:28:23Z
The warden identified a flaw in the logic, the sponsor confirms
🌟 Selected for report: pauliax
367.919 USDC - $367.92
pauliax
These checks should be inclusive:
require(amountOut > minOut, "EarlyExit: Insufficient output"); require(_bps > 0 && _bps < 1000, "Must be between 0-100%"); require(newThreshold > 0 && newThreshold < 10000, "Threshold must be between 0-100%"); require(_distance > 0 && _distance < 1000, "Override must be between 0-100%");
Replace > with >= and < with <= where necesseary.
#0 - GalloDaSballo
2022-01-24T00:29:18Z
The warden identified a slight incongruence between the documentation and the code. Finding is valid
🌟 Selected for report: Meta0xNull
Also found by: pauliax
pauliax
_setupRole is used across the contracts, e.g. in function addNewBuyer, assignRole or initialize. This is not a recommended approach. See the warning: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.1.0/contracts/access/AccessControl.sol#L183-L189
OZ suggests using _grantRole when you need to assign roles not from the constructor.
#0 - 0xScotch
2021-12-08T15:50:21Z
#115
#1 - GalloDaSballo
2022-01-24T00:36:56Z
Duplicate of #115
🌟 Selected for report: pauliax
367.919 USDC - $367.92
pauliax
In functions transferAndCall and transferWithPermit the condition should be AND, not OR:
require(to != address(0) || to != address(this));
require(to != address(0) && to != address(this));
#0 - GalloDaSballo
2022-01-24T00:38:07Z
Agree with finding
🌟 Selected for report: pauliax
367.919 USDC - $367.92
pauliax
When auction.endingTime == now, function purchaseArbitrageTokens thinks that auction is still active, while isAuctionFinished and earlyExitReturn think that it has ended: purchaseArbitrageTokens:
require(auction.endingTime >= now, "Auction is already over");
isAuctionFinished:
return auction.endingTime > 0 && (now >= auction.endingTime || ...);
earlyExitReturn:
if(active || block.timestamp < auctionEndTime) { return 0; }
Consider unifying it across the functions.
#0 - GalloDaSballo
2022-01-24T00:35:26Z
The warden has identified an inconsistency from what seems to be the same logical state in the finite state machine of the contracts. I agree with the finding, and the sponsor confirms as well
165.5635 USDC - $165.56
pauliax
The variable DOMAIN_SEPARATOR in contract ERC20Permit is assigned in the constructor and will not change after being initialized. However, if a hard fork happens after the contract deployment, the domain would become invalid on one of the forked chains due to the block.chainid has changed. Also, you don't need an assmebly to retrieve chainid, you can get it from a built in variable block.chainid.
Similar issues were reported in a previous contest and were assigned a severity of low: https://github.com/code-423n4/2021-06-realitycards-findings/issues/166 https://github.com/code-423n4/2021-09-swivel-findings/issues/98
An elegant solution that you may consider applying is from Sushi Trident: https://github.com/sushiswap/trident/blob/concentrated/contracts/pool/concentrated/TridentNFT.sol#L47-L62
#0 - GalloDaSballo
2022-01-24T00:38:37Z
Agree with the finding and severity
🌟 Selected for report: pmerkleplant
Also found by: 0x0x0x, GiveMeTestEther, WatchPug, pauliax, robee, ye0lde
pauliax
.length in a loop can be extracted into a variable and used where necessary to reduce the number of storage reads Cache the length of the array and use this local variable when iterating over the storage array, e.g.:
for (uint256 i = replenishingIndex; i < auctionIds.length; i = i + 1) for (uint i = 0; i < verifierList.length - 1; i = i + 1) for (uint i = 0; i < mines.length; i = i + 1)
#0 - 0xScotch
2021-12-08T18:39:14Z
#106
#1 - GalloDaSballo
2022-01-18T13:23:32Z
Duplicate of #106
🌟 Selected for report: pauliax
367.919 USDC - $367.92
pauliax
There are several loops in the contract which can eventually grow so large as to make future operations of the contract cost too much gas to fit in a block, e.g.:
for (uint256 i = replenishingIndex; i < auctionIds.length; i = i + 1) // function outstandingArbTokens() while (true) // function allocateArbRewards
Consider introducing a reasonable upper limit based on block gas limits. Also, you can consider using EnumerableSet (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/EnumerableSet.sol) where possible, e.g. 'buyers' or 'verifierList'.
#0 - GalloDaSballo
2022-01-18T14:56:43Z
Agree with the finding, would have preferred a list of occurencies though as this message doesn't help the sponsor mitigate
pauliax
No need to check if the value is >= 0, as uint can never go negative:
require(_profitCut >= 0 require(_delay >= 0
#0 - 0xScotch
2021-12-08T18:16:29Z
#309
#1 - GalloDaSballo
2021-12-30T16:15:11Z
Duplicate of #309
15.2616 USDC - $15.26
pauliax
Consider caching decimals when initializing malt and collateralToken to avoid repeated external calls, as they are not supposed to change unless initialized again:
uint256 maltDecimals = malt.decimals(); uint256 decimals = collateralToken.decimals();
#0 - GalloDaSballo
2021-12-30T16:56:41Z
Agree with finding
15.2616 USDC - $15.26
pauliax
You do not need SafeMath operations where it is not possible to overflow/underflow, e.g.:
if (maltUsed < maltBalance) { malt.safeTransfer(msg.sender, maltBalance.sub(maltUsed)); } if (rewardUsed < rewardBalance) { rewardToken.safeTransfer(msg.sender, rewardBalance.sub(rewardUsed)); } require(forfeited <= _globals.declaredBalance, "Cannot forfeit more than declared"); _globals.declaredBalance = _globals.declaredBalance.sub(forfeited);
#0 - 0xScotch
2021-12-08T18:33:34Z
#307
#1 - GalloDaSballo
2021-12-31T19:13:30Z
Duplicate of #307
🌟 Selected for report: pauliax
56.5245 USDC - $56.52
pauliax
contract PoolTransferVerification sets thresholdBps but in calculations uses only (10000 - thresholdBps). Consider pre-calculating to avoid re-evaluation again and again when this function is invoked.
#0 - GalloDaSballo
2021-12-31T17:10:01Z
Agree with the finding, a simple setter would do and saves the extra SUB
pauliax
function purchaseArbitrageTokens unnecessarily checks if the auction is active twice:
require(auctionActive(currentAuctionId), "No auction running"); require(auction.active == true, "Auction is not active");
#0 - 0xScotch
2021-12-08T18:34:41Z
#108
#1 - GalloDaSballo
2021-12-31T19:16:08Z
Duplicate of #108
🌟 Selected for report: GiveMeTestEther
Also found by: pauliax
pauliax
It would be cheapier if you emit events using local (memory) variables, not read from storage, e.g.:
emit NewDelay(delay); // _delay emit NewGracePeriod(gracePeriod); // _gracePeriod
#0 - 0xScotch
2021-12-08T18:44:47Z
#214
#1 - GalloDaSballo
2021-12-31T19:44:16Z
Duplicate of #214
10.3016 USDC - $10.30
pauliax
In loops you use this incrementing approach:
i = i + 1
Incrementing values is cheapest by doing i++.
#0 - 0xScotch
2021-12-09T22:04:24Z
#295
#1 - GalloDaSballo
2021-12-31T19:55:33Z
Duplicate of #295
🌟 Selected for report: pauliax
pauliax
You don't need to import the implementation to interact with the contract, you can import only an interface, e.g. here:
ERC20 public collateralToken;
Consider replacing ERC20 with IERC20 to reduce deployment costs.
#0 - 0xScotch
2021-12-08T18:43:57Z
#242
#1 - GalloDaSballo
2021-12-31T19:43:15Z
This is not a duplicate of #242, whereas 242 is about Best Practices, this finding is saying that using ERC20 instead of IERC20 will increase the cost of the deployment.
I believe the optimizer will make the bytecode the same, but will mark this finding as valid because IERC20 would guarantee less work
165.5635 USDC - $165.56
pauliax
Function purchaseArbitrageTokens should validate that amount > 0, otherwise it may be possible to spam accountCommitmentEpochs with 0 amounts:
if (auction.accountCommitments[msg.sender].commitment == 0) { accountCommitmentEpochs[msg.sender].push(currentAuctionId); }
require amount > 0
#0 - GalloDaSballo
2022-01-18T14:55:51Z
Agree with the finding