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: 10/22
Findings: 3
Award: $167.73
🌟 Selected for report: 1
🚀 Solo Findings: 0
69.1238 USDC - $69.12
throttle
Price can be stale and can lead to wrong quoteAmount return value
Oracle data feed is insufficiently validated. There is no check for stale price and round completeness.
Price can be stale and can lead to wrong quoteAmount return value
function _peek( bytes6 base, bytes6 quote, uint256 baseAmount ) private view returns (uint256 quoteAmount, uint256 updateTime) { ... (, int256 daiPrice, , , ) = DAI.latestRoundData(); (, int256 usdcPrice, , , ) = USDC.latestRoundData(); (, int256 usdtPrice, , , ) = USDT.latestRoundData(); require( daiPrice > 0 && usdcPrice > 0 && usdtPrice > 0, "Chainlink pricefeed reporting 0" ); ... }
Manual review
Validate data feed
function _peek( bytes6 base, bytes6 quote, uint256 baseAmount ) private view returns (uint256 quoteAmount, uint256 updateTime) { ... (uint80 roundID, int256 daiPrice, , uint256 timestamp, uint80 answeredInRound) = DAI.latestRoundData(); require(daiPrice > 0, "ChainLink: DAI price <= 0"); require(answeredInRound >= roundID, "ChainLink: Stale price"); require(timestamp > 0, "ChainLink: Round not complete"); (roundID, int256 usdcPrice, , timestamp, answeredInRound) = USDC.latestRoundData(); require(usdcPrice > 0, "ChainLink: USDC price <= 0"); require(answeredInRound >= roundID, "ChainLink: Stale USDC price"); require(timestamp > 0, "ChainLink: USDC round not complete"); (roundID, int256 usdtPrice, , timestamp, answeredInRound) = USDT.latestRoundData(); require(usdtPrice > 0, "ChainLink: USDT price <= 0"); require(answeredInRound >= roundID, "ChainLink: Stale USDT price"); require(timestamp > 0, "ChainLink: USDT round not complete"); ... }
#0 - GalloDaSballo
2022-02-14T22:33:14Z
When using Chainlink Price feeds it is important to ensure the price feed data was updated recently. While getting started with chainlink requires just one line of code, it is best to add additional checks for in production environments.
I believe the finding to be valid and Medium severity to be appropriate.
The sponsor has mitigated in a subsequent PR
42.7547 USDC - $42.75
throttle
Gas savings
Function ConvexYieldWrapper.removeVault() can be rewritten. As such, 1 variable can be removed and 1 MLOAD, 1 MSTORE can be saved.
Manual review
Rewrite this:
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_; } }
To this
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(); vaults[account] = vaults_; emit VaultRemoved(account, vaultId); return; } } revert("Vault not found"); } }
#0 - alcueca
2022-02-02T16:52:15Z
Duplicate #111
throttle
Gas savings
Some variables can be set immutable: https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexYieldWrapper.sol#L17 https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexStakingWrapper.sol#L41-L45
Manual review
Change to immutable
#0 - devtooligan
2022-02-01T02:22:51Z
dup of #42
17.3157 USDC - $17.32
throttle
Gas savings
Reentrancy modifier is costly. Setting state variables to uint 0/1 is cheaper https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexStakingWrapper.sol#L54-L55
Manual review
Change to
uint256 private constant _NOT_ENTERED = 0; uint256 private constant _ENTERED = 1;
#0 - devtooligan
2022-01-31T22:26:06Z
interesting
#1 - alcueca
2022-02-02T16:57:58Z
Duplicate of #96, that has a better solution
25.6528 USDC - $25.65
throttle
Gas savings
Assigning default value costs unnecessary gas: ConvexStakingWrapper._calcCvxIntegral() ConvexStakingWrapper._calcRewardIntegral() ConvexStakingWrapper._checkpointAndClaim() ConvexStakingWrapper.earned() ConvexYieldWrapper.removeVault() ConvexYieldWrapper._getDepositedBalance()
Manual review
Consider rewriting
for (uint256 i = 0; i < k; i++) { ... }
to
for (uint256 i; i < k; i++) { ... }
#0 - devtooligan
2022-01-31T22:29:48Z
dup of #15
#1 - alcueca
2022-02-02T16:16:00Z
Duplicate of #56, better
throttle
Gas savings
Using pre-increment instead of post-increment can save gas in following places:
ConvexStakingWrapper.addRewards() ConvexStakingWrapper._calcCvxIntegral() ConvexStakingWrapper._calcRewardIntegral() ConvexStakingWrapper._checkpointAndClaim() ConvexStakingWrapper.earned() ConvexYieldWrapper.removeVault() ConvexYieldWrapper._getDepositedBalance()
Manual review
Change i++ to ++i
#0 - devtooligan
2022-01-31T22:45:28Z
dup of #14