Yield-Convex contest - Dravee's results

Handle

Dravee

Vulnerability details

Impact

No protection from reentrancy (besides the gas limit on safeTransfer). Bad practice compared to the original ConvexStakingWrapper contract.

Proof of Concept

The original ConvexStakingWrapper contract used the nonReentrant modifier on all functions using the safeTransfer or safeTransferFrom methods:

• deposit: https://github.com/convex-eth/platform/blob/main/contracts/contracts/wrappers/ConvexStakingWrapper.sol#L337
• stake: https://github.com/convex-eth/platform/blob/main/contracts/contracts/wrappers/ConvexStakingWrapper.sol#L352
• withdraw: https://github.com/convex-eth/platform/blob/main/contracts/contracts/wrappers/ConvexStakingWrapper.sol#L367
• withdrawAndUnwrap: https://github.com/convex-eth/platform/blob/main/contracts/contracts/wrappers/ConvexStakingWrapper.sol#L381

As the current one in the Yield solution is an upgrade, it should follow the same good practices.

Tools Used

VS Code

Recommended Mitigation Steps

Use the nonReentrant modifier on external functions that end up calling safeTransfer or safeTransferFrom (user_checkpoint() and getReward())

Handle

Dravee

Vulnerability details

Impact

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Proof of Concept

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

ConvexStakingWrapper.sol:115:        for (uint256 i = startIndex; i < extraCount; i++) {
ConvexStakingWrapper.sol:172:        for (uint256 u = 0; u < accountsLength; u++) {
ConvexStakingWrapper.sol:227:        for (uint256 u = 0; u < accountsLength; u++) {
ConvexStakingWrapper.sol:271:        for (uint256 i = 0; i < rewardCount; i++) {
ConvexStakingWrapper.sol:287:        for (uint256 i = 0; i < rewardCount; i++) {
ConvexStakingWrapper.sol:315:        for (uint256 i = 0; i < rewardCount; i++) {
ConvexYieldWrapper.sol:63:        for (uint256 i = 0; i < vaultsLength; i++) {
ConvexYieldWrapper.sol:80:            for (uint256 i = 0; i < vaultsLength; i++) {
ConvexYieldWrapper.sol:111:        for (uint256 i = 0; i < userVaultLength; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Use ++i instead of i++ to increment the value of an uint variable.

Handle

Dravee

Vulnerability details

Impact

SLOADs are expensive (~100 gas) compared to MLOADs/MSTOREs (~3 gas). Minimizing them can save gas.

Proof of Concept

The code is as such (see @audit-info):

File: Cvx3CrvOracle.sol
110:     function _peek(
111:         bytes6 base,
112:         bytes6 quote,
113:         uint256 baseAmount
114:     ) private view returns (uint256 quoteAmount, uint256 updateTime) {
115:         require(
116:             (base == ethId && quote == cvx3CrvId) || // @audit-info ethId SLOAD 1, cvx3CrvId SLOAD 1
117:                 (base == cvx3CrvId && quote == ethId), // @audit-info ethId SLOAD 2, cvx3CrvId SLOAD 2
118:             "Invalid quote or base"
119:         );
120:         (, int256 daiPrice, , , ) = DAI.latestRoundData();
121:         (, int256 usdcPrice, , , ) = USDC.latestRoundData();
122:         (, int256 usdtPrice, , , ) = USDT.latestRoundData();
123: 
124:         require(
125:             daiPrice > 0 && usdcPrice > 0 && usdtPrice > 0,
126:             "Chainlink pricefeed reporting 0"
127:         );
128: 
129:         // This won't overflow as the max value for int256 is less than the max value for uint256
130:         uint256 minStable = min(
131:             uint256(daiPrice),
132:             min(uint256(usdcPrice), uint256(usdtPrice))
133:         );
134: 
135:         uint256 price = (threecrv.get_virtual_price() * minStable) / 1e18;
136: 
137:         if (base == cvx3CrvId && quote == ethId) { // @audit-info ethId SLOAD 3, cvx3CrvId SLOAD 3
138:             quoteAmount = (baseAmount * price) / 1e18;
139:         } else {
140:             quoteAmount = (baseAmount * 1e18) / price;
141:         }
142: 
143:         updateTime = block.timestamp;
144:     }

By caching ethId and cvx3CrvId in memory, it's possible to save 4 SLOADs (~400gas) at the cost of 2 MSTOREs (6 gas) and 4 MLOADs (12 gas)

Tools Used

VS Code

Recommended Mitigation Steps

Cache ethId and cvx3CrvId in variables and use these instead

Handle

Dravee

Vulnerability details

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

Instances include:

ConvexStakingWrapper.sol:54:    bool private constant _NOT_ENTERED = false;
ConvexStakingWrapper.sol:172:        for (uint256 u = 0; u < accountsLength; u++) {
ConvexStakingWrapper.sol:227:        for (uint256 u = 0; u < accountsLength; u++) {
ConvexStakingWrapper.sol:271:        for (uint256 i = 0; i < rewardCount; i++) {
ConvexStakingWrapper.sol:287:        for (uint256 i = 0; i < rewardCount; i++) {
ConvexStakingWrapper.sol:315:        for (uint256 i = 0; i < rewardCount; i++) {
ConvexYieldWrapper.sol:63:        for (uint256 i = 0; i < vaultsLength; i++) {
ConvexYieldWrapper.sol:80:            for (uint256 i = 0; i < vaultsLength; i++) {
ConvexYieldWrapper.sol:111:        for (uint256 i = 0; i < userVaultLength; i++) {

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove explicit initialization for default values.

Handle

Dravee

Vulnerability details

Impact

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

Proof of Concept

In ConvexStakingWrapper.sol, the order of variables is this way:

    uint256 public cvx_reward_integral;
    uint256 public cvx_reward_remaining;
    mapping(address => uint256) public cvx_reward_integral_for;
    mapping(address => uint256) public cvx_claimable_reward;

    //constants/immutables
    address public constant convexBooster = address(0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
    address public constant crv = address(0xD533a949740bb3306d119CC777fa900bA034cd52);
    address public constant cvx = address(0x4e3FBD56CD56c3e72c1403e103b45Db9da5B9D2B);
    address public curveToken;
    address public convexToken;
    address public convexPool;
    address public collateralVault;
    uint256 public convexPoolId;

    //rewards
    RewardType[] public rewards;

    //management
    bool public isShutdown;
    bool private _status;

    bool private constant _NOT_ENTERED = false;
    bool private constant _ENTERED = true;

address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous). As bool type variables are of size 1 byte, there's a slot here that can get saved by moving them closer to an address

Recommended Mitigation Steps

I suggest the following (see the @audit-info tags for more details about what moved and why):

    uint256 public cvx_reward_integral;
    uint256 public cvx_reward_remaining;
    mapping(address => uint256) public cvx_reward_integral_for;
    mapping(address => uint256) public cvx_claimable_reward;

    //constants/immutables
    uint256 public convexPoolId; //@audit-info this moved up to free collateralVault's slot.
    address public constant convexBooster = address(0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
    address public constant crv = address(0xD533a949740bb3306d119CC777fa900bA034cd52);
    address public constant cvx = address(0x4e3FBD56CD56c3e72c1403e103b45Db9da5B9D2B);
    address public curveToken;
    address public convexToken;
    address public convexPool;
    address public collateralVault; //@audit-info this got freed from convexPoolId. Slot N is at 20/32 here

    //management 
    bool public isShutdown; //@audit-info this moved up. Slot N is full at 21/32 here 
    bool private _status; //@audit-info this moved up. Slot N is full at 22/32 here

    bool private constant _NOT_ENTERED = false; //@audit-info this moved up but doesn't take a slot as it's constant
    bool private constant _ENTERED = true; //@audit-info this moved up but doesn't take a slot as it's constant
	
    //rewards
    RewardType[] public rewards;

Handle

Dravee

Vulnerability details

Impact

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof of Concept

> 0 is used in the following location(s):

ConvexStakingWrapper.sol:165:        if (_supply > 0 && d_cvxreward > 0) {
ConvexStakingWrapper.sol:182:                    if (receiveable > 0) {
ConvexStakingWrapper.sol:221:        if (_supply > 0 && (bal - rewardRemaining) > 0) {
ConvexStakingWrapper.sol:237:                    if (receiveable > 0) {
ConvexStakingWrapper.sol:325:            if (supply > 0) {
ConvexYieldWrapper.sol:128:        require(amount_ > 0, "No convex token to wrap");
ConvexYieldWrapper.sol:142:        require(amount_ > 0, "No wrapped convex token");

Tools Used

VS Code

Recommended Mitigation Steps

Change > 0 with != 0.

Handle

Dravee

Vulnerability details

Impact

Increased gas cost due to unnecessary automatic underflow checks.

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers.

When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block.

https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Proof of Concept

In ConvexStakingWrapper.sol:_calcRewardIntegral(), bal - rewardRemaining can't underflow at line 222 as the conditional statement line 221 prevents it:

File: ConvexStakingWrapper.sol
221:         if (_supply > 0 && (bal - rewardRemaining) > 0) {
222:             rewardIntegral = uint128(rewardIntegral) + uint128(((bal - rewardRemaining) * 1e20) / _supply); //@audit-info (bal - rewardRemaining) can't underflow because of above if statement

This substraction should get computed inside an unchecked block and stored in a variable, which would then be used in the checked calculation for rewardIntegral.

Tools Used

VS Code

Recommended Mitigation Steps

Uncheck arithmetic operations when the risk of underflow or overflow is already contained by wrapping them in an unchecked block