Platform: Code4rena
Start Date: 28/01/2022
Pot Size: $30,000 USDC
Total HM: 4
Participants: 22
Period: 3 days
Judge: GalloDaSballo
Total Solo HM: 2
Id: 80
League: ETH
Rank: 3/22
Findings: 4
Award: $3,010.91
🌟 Selected for report: 6
🚀 Solo Findings: 1
69.1238 USDC - $69.12
WatchPug
function _peek( bytes6 base, bytes6 quote, uint256 baseAmount ) private view returns (uint256 quoteAmount, uint256 updateTime) { require( (base == ethId && quote == cvx3CrvId) || (base == cvx3CrvId && quote == ethId), "Invalid quote or base" ); (, int256 daiPrice, , , ) = DAI.latestRoundData(); (, int256 usdcPrice, , , ) = USDC.latestRoundData(); (, int256 usdtPrice, , , ) = USDT.latestRoundData(); require( daiPrice > 0 && usdcPrice > 0 && usdtPrice > 0, "Chainlink pricefeed reporting 0" );
On Cvx3CrvOracle.sol, we are using Chainlink's latestRoundData API, but there is no check if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation:
Consider adding missing checks for stale data.
For example:
(uint80 daiRoundID, int256 daiPrice, , uint256 daiTimestamp, uint80 daiAnsweredInRound) = DAI.latestRoundData(); require(daiPrice > 0, "DAI: Chainlink price <= 0"); require(daiAnsweredInRound >= daiRoundID, "DAI: Stale price"); require(daiTimestamp != 0, "DAI: Round not complete"); (uint80 usdcRoundID, int256 usdcPrice, , uint256 usdcTimestamp, uint80 usdcAnsweredInRound) = USDC.latestRoundData(); require(usdcPrice > 0, "USDC: Chainlink price <= 0"); require(usdcAnsweredInRound >= usdcRoundID, "USDC: Stale price"); require(usdcTimestamp != 0, "USDC: Round not complete"); (uint80 usdtRoundID, int256 usdtPrice, , uint256 usdtTimestamp, uint80 usdtAnsweredInRound) = USDT.latestRoundData(); require(usdtPrice > 0, "USDT: Chainlink price <= 0"); require(usdtAnsweredInRound >= usdtRoundID, "USDT: Stale price"); require(usdtTimestamp != 0, "USDT:Round not complete");
#0 - iamsahu
2022-02-01T19:27:05Z
#136 And the suggested solution would lead to a stack too deep error.
🌟 Selected for report: WatchPug
2643.2694 USDC - $2,643.27
WatchPug
function _calcRewardIntegral( uint256 _index, address[2] memory _accounts, uint256[2] memory _balances, uint256 _supply, bool _isClaim ) internal { RewardType storage reward = rewards[_index]; uint256 rewardIntegral = reward.reward_integral; uint256 rewardRemaining = reward.reward_remaining; //get difference in balance and remaining rewards //getReward is unguarded so we use reward_remaining to keep track of how much was actually claimed uint256 bal = IERC20(reward.reward_token).balanceOf(address(this)); if (_supply > 0 && (bal - rewardRemaining) > 0) { rewardIntegral = uint128(rewardIntegral) + uint128(((bal - rewardRemaining) * 1e20) / _supply); reward.reward_integral = uint128(rewardIntegral); }
reward.reward_integral is uint128, if a early user mint (wrap) just 1 Wei of convexToken, and make _supply == 1, and then tranferring 5e18 of reward_token to the contract.
As a result, reward.reward_integral can exceed type(uint128).max and overflow, causing the rewards distribution to be disrupted.
Consider wrap a certain amount of initial totalSupply, e.g. 1e8, and never burn it. And consider using uint256 instead of uint128 for reward.reward_integral. Also, consdier lower 1e20 down to 1e12.
#0 - alcueca
2022-02-02T15:33:25Z
Assets are not a direct risk if it is the first user disrupting the contract. We would need to redeploy better code, but that's it. I suggest this is reported as medium, as merits for a DoS attack.
As suggested elsewhere, the right solution if uint128 is to be used in storage, is to cast up to uint256 for calculations, and then back to uint128 to store again.
#1 - GalloDaSballo
2022-02-18T00:55:26Z
The warden identified a way an early depositor can grief the system, I believe the finding to be valid, and because:
I believe medium severity to be appropriate
42.7547 USDC - $42.75
WatchPug
function removeVault(bytes12 vaultId, address account) public { address owner = cauldron.vaults(vaultId).owner; if (account != owner) { bytes12[] storage vaults_ = vaults[account]; uint256 vaultsLength = vaults_.length; bool found; for (uint256 i = 0; i < vaultsLength; i++) { if (vaults_[i] == vaultId) { bool isLast = i == vaultsLength - 1; if (!isLast) { vaults_[i] = vaults_[vaultsLength - 1]; } vaults_.pop(); found = true; emit VaultRemoved(account, vaultId); break; } } require(found, "Vault not found"); vaults[account] = vaults_; } }
found is redundant, we can just use return to stop the whole function when the vault to be removed is found and removed.
removeVault() can be changed to:
function removeVault(bytes12 vaultId, address account) public { address owner = cauldron.vaults(vaultId).owner; if (account != owner) { bytes12[] storage vaults_ = vaults[account]; uint256 vaultsLength = vaults_.length; for (uint256 i = 0; i < vaultsLength; i++) { if (vaults_[i] == vaultId) { bool isLast = i == vaultsLength - 1; if (!isLast) { vaults_[i] = vaults_[vaultsLength - 1]; } vaults_.pop(); found = true; emit VaultRemoved(account, vaultId); return; } } revert("Vault not found"); } }
#0 - GalloDaSballo
2022-02-11T02:34:31Z
The sponsor confirmed and implemented the finding
42.7547 USDC - $42.75
WatchPug
function removeVault(bytes12 vaultId, address account) public { address owner = cauldron.vaults(vaultId).owner; if (account != owner) { bytes12[] storage vaults_ = vaults[account]; uint256 vaultsLength = vaults_.length; bool found; for (uint256 i = 0; i < vaultsLength; i++) { if (vaults_[i] == vaultId) { bool isLast = i == vaultsLength - 1; if (!isLast) { vaults_[i] = vaults_[vaultsLength - 1]; } vaults_.pop(); found = true; emit VaultRemoved(account, vaultId); break; } } require(found, "Vault not found"); vaults[account] = vaults_; } }
At L77, vaults_ is defined as vaults[account], thus vaults[account] = vaults_ at L93 is redundant.
function addVault(bytes12 vaultId) external { address account = cauldron.vaults(vaultId).owner; require(account != address(0), "No owner for the vault"); bytes12[] storage vaults_ = vaults[account]; uint256 vaultsLength = vaults_.length; for (uint256 i = 0; i < vaultsLength; i++) { require(vaults_[i] != vaultId, "Vault already added"); } vaults_.push(vaultId); vaults[account] = vaults_; emit VaultAdded(account, vaultId); }
Similarly, L76 is redundant.
#0 - GalloDaSballo
2022-02-11T02:39:07Z
The sponsor has implemented the improvement
🌟 Selected for report: WatchPug
95.0104 USDC - $95.01
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:
uint256 startIndex = rewardsLength - 1;
rewardsLength - 1 will never underflow.
bool isLast = i == vaultsLength - 1; if (!isLast) { vaults_[i] = vaults_[vaultsLength - 1]; }
#0 - iamsahu
2022-02-01T14:49:54Z
From readme:
#1 - GalloDaSballo
2022-02-11T02:40:52Z
Interestingly enough, while the specific link mentioned by the warden wouldn't save 100 gas, using unchecked on that code and the for loop would.
While I'm not a fan of unchecked due to syntax, I think the finding is valid and above the sponsors require of at last 100 gas
#2 - alcueca
2022-02-21T09:16:23Z
Fair enough, thank you.
WatchPug
address public curveToken; address public convexToken; address public convexPool; address public collateralVault; uint256 public convexPoolId;
Considering that curveToken, convexToken, convexPool, and convexPoolId will never change, changing them to immutable variables instead of storages variable can save gas.
Other examples include:
bytes32 public cvx3CrvId; bytes32 public ethId;
cvx3CrvId and ethId in Cvx3CrvOracle.sol.
#0 - devtooligan
2022-02-01T02:27:13Z
dup #42 and also cant make bytes immutable
#1 - GalloDaSballo
2022-02-11T02:46:36Z
Duplicate of #42
17.3157 USDC - $17.32
WatchPug
bool private constant _NOT_ENTERED = false; bool private constant _ENTERED = true;
modifier nonReentrant() { // On the first call to nonReentrant, _notEntered will be true require(_status != _ENTERED, "ReentrancyGuard: reentrant call"); // Any calls to nonReentrant after this point will fail _status = _ENTERED; _; // By storing the original value once again, a refund is triggered (see // https://eips.ethereum.org/EIPS/eip-2200) _status = _NOT_ENTERED; }
SSTORE from 0 to 1 (or any non-zero value), the cost is 20000;
SSTORE from 1 to 2 (or any other non-zero value), the cost is 5000.
By storing the original value once again, a refund is triggered (https://eips.ethereum.org/EIPS/eip-2200).
Since refunds are capped to a percentage of the total transaction's gas, it is best to keep them low, to increase the likelihood of the full refund coming into effect.
Therefore, switching between 1, 2 instead of 0, 1 will be more gas efficient.
#0 - GalloDaSballo
2022-02-11T02:47:32Z
The finding is valid, changing from non-zero to non-zero is indeed cheaper
🌟 Selected for report: WatchPug
95.0104 USDC - $95.01
WatchPug
if (rewardsLength == 0) { RewardType storage reward = rewards.push(); reward.reward_token = crv; reward.reward_pool = mainPool; rewardsLength += 1; }
When rewardsLength == 0, the new rewardsLength will always be 1. Therefore, replacing += with = can avoid the unnecessary arithmetic operations and memory reads
Change to:
if (rewardsLength == 0) { RewardType storage reward = rewards.push(); reward.reward_token = crv; reward.reward_pool = mainPool; rewardsLength = 1; }
#0 - GalloDaSballo
2022-02-13T01:46:39Z
Finding is valid and the sponsor used it, notice that this should save 3 gas as it's avoiding an MLOAD