Skip to content

Latest commit

 

History

History
330 lines (217 loc) · 19.2 KB

low.md

File metadata and controls

330 lines (217 loc) · 19.2 KB
Raw

Report

Summary

Low Issues

Total of 18 issues:

ID Issue
L-1 Hardcoded addresses may not be compatible with a multi-chain deployment
L-2 Use Ownable2Step instead of Ownable for access control
L-3 Missing name and symbol for AfEth token
L-4 Validate ratio argument in setRatio()
L-5 Price calculations may experience precision loss due to division before multiplication
L-6 Validate withdrawals in AfEth have been already executed
L-7 Prevent AfEth token transfers to its own contract
L-8 Public relock function can be used to grief withdrawal requests
L-9 Missing call to base initializers
L-10 Missing check for zero address value in constructor or setter
L-11 Function canWithdraw() in VotiumStrategy doesn't check if withdrawal has been already executed
L-12 VotiumStrategy allows to recover ERC20 tokens but not native ETH
L-13 Missing usage of safe wrappers to handle ERC20 operations
L-14 Zero token allowance can cause denial of service in applyRewards()
L-15 Low level calls to account with no code will not fail
L-16 Protocol fees are not collected when rewards are not routed through AfEth
L-17 Protocol doesn't collect fees from SafEth
L-18 Potential rounding to zero issue in AfEth deposit could cause loss of value

Non Critical Issues

Total of 4 issues:

ID Issue
NC-1 Remove debug symbols
NC-2 Missing event for important parameter change
NC-3 Unused constants
NC-4 Use constants for literal or magic values

Informational Issues

Total of 1 issues:

ID Issue
INFO-1 Consider using the ERC4626 standard

Low Issues

[L-1] Hardcoded addresses may not be compatible with a multi-chain deployment

The codebase is full of hardcoded addresses that refer to other parts of the protocol (SafEth, for example) or third-party protocols (e.g. Votium, Convex, Curve Pools).

Assumptions of the presence of third-party protocol, their deployment addresses and/or their arguments will prevent or complicate deployments in layer 2 chains. This is stated as a possibility in the documentation:

This will only be deployed to Ethereum Mainnet, with the chance of being deployed on L2's on a future date

[L-2] Use Ownable2Step instead of Ownable for access control

Use the Ownable2Step variant of the Ownable contract to better safeguard against accidental transfers of access control.

Instances (2):

[L-3] Missing name and symbol for AfEth token

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L72

The AfEth contract inherits from ERC20Upgradeable but doesn't call its base initializer, which is in charge of setting the name and symbol for the ERC20 token.

[L-4] Validate ratio argument in setRatio()

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L90

The ratio configuration parameter in AfEth measures the portion of deposited value that goes into the SafEth contract. This value is intended to range between 0 (0%) and 1e18 (100%).

The setRatio() function should validate that the new value is within bounds, i.e. require(_newRatio <= 1e18).

[L-5] Price calculations may experience precision loss due to division before multiplication

There are several places across the codebase in which intermediate results that depend on a division are then used as part of calculations that involve a multiple.

For example, in requestWithdraw() the calculation of votiumWithdrawAmount depends on the intermediate result of withdrawRatio. The expression can be simplified as:

uint256 votiumWithdrawAmount = (_amount * votiumBalance) / (totalSupply() - afEthBalance);

And similarly, safEthWithdrawAmount:

uint256 safEthWithdrawAmount = (_amount * safEthBalance) / (totalSupply() - afEthBalance);

[L-6] Validate withdrawals in AfEth have been already executed

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L243

Withdrawals in AfEth are first enqueued when requested, and executed later when the vAfEth tokens are ready to be withdrawn. The withdraw() function in AfEth validates that the withdrawal is ready (by using canWithdraw()) but doesn't validate if the withdrawal has been already executed.

This isn't currently exploitable since the withdrawal in AfEth also depends on the withdrawal in VotiumStrategy, which correctly checks if the withdrawal associated to the vEthWithdrawId has been already claimed. Consider adding a similar check to AfEth::withdraw().

[L-7] Prevent AfEth token transfers to its own contract

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L183

When a user requests a withdrawal in AfEth, their tokens are transferred to the contract and "locked" until the withdrawal is made effective, at which point the tokens are burned.

Given this mechanism, AfEth tokens held by the same contract are considered as locked tokens but anyone can technically transfer tokens to the contract by calling transfer() or transferFrom().

Consider adding a check to the base ERC20 functionality to prevent AfEth tokens from being sent to the contract by external actors.

[L-8] Public relock function can be used to grief withdrawal requests

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L135

A malicious user can call relock() to front-run a transaction to requestWithdraw().

Relocking will take any available CVX in the contract and expired locks in CvxLocker and relock them. Griefed users will need to wait more, as any of the available balance that could have been used for the withdrawal has been relocked as part of the front-running.

[L-9] Missing call to base initializers

Upgradeable contracts that inherit from other base contracts should call the corresponding base initializers during initialization.

Instances (4):

[L-10] Missing check for zero address value in constructor or setter

Address parameters should be validated to guard against the default value address(0).

Instances (6):

[L-11] Function canWithdraw() in VotiumStrategy doesn't check if withdrawal has been already executed

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L155

The implementation of canWithdraw() in the VotiumStrategy contract just checks if the required epoch has been reached, but doesn't validate if the request has been already fulfilled (WithdrawRequestInfo.withdrawn).

Consider also checking for the withdrawn condition as part of the implementation of canWithdraw().

function canWithdraw(
    uint256 _withdrawId
) external view virtual override returns (bool) {
    uint256 currentEpoch = ILockedCvx(VLCVX_ADDRESS).findEpochId(
        block.timestamp
    );
    return
        withdrawIdToWithdrawRequestInfo[_withdrawId].epoch <= currentEpoch && !withdrawIdToWithdrawRequestInfo[_withdrawId].withdrawn;
}

[L-12] VotiumStrategy allows to recover ERC20 tokens but not native ETH

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L215

The implementation of withdrawStuckTokens() allows the owner of the protocol to recover any ERC20 token, but fails to consider native ETH transfers.

[L-13] Missing usage of safe wrappers to handle ERC20 operations

ERC20 operations on arbitrary tokens should be safely wrapped to account for incompatible implementations.

[L-14] Zero token allowance can cause denial of service in applyRewards()

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L282-L285

IERC20(_swapsData[i].sellToken).approve(
    address(_swapsData[i].spender),
    0
);

During applyRewards(), token allowances are first reset to zero before being increased to infinity. This could cause issues with some ERC20 implementations that revert on zero value approvals, such as BNB.

[L-15] Low level calls to account with no code will not fail

Low level calls (i.e. address.call(...)) to account with no code will silently succeed without reverting or throwing any error. Quoting the reference for the CALL opcode in evm.codes:

Creates a new sub context and execute the code of the given account, then resumes the current one. Note that an account with no code will return success as true.

[L-16] Protocol fees are not collected when rewards are not routed through AfEth

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L302-L304

Protocol fees are collected in AfEth::depositRewards(), just before rewards are being compounded in the protocol.

The reward flow is initiated in VotiumStrategyCore::applyRewards(), and will deposit rewards in AfEth only if the manager address is defined:

302:         if (address(manager) != address(0))
303:             IAfEth(manager).depositRewards{value: ethReceived}(ethReceived);
304:         else depositRewards(ethReceived);

It is important to note that if rewards aren't channeled through AfEth, the protocol will not receive any fees.

[L-17] Protocol doesn't collect fees from SafEth

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L272

Protocol fees are only collected as part of the deposited rewards coming from the VotiumStrategy contract. Votium and Convex rewards are claimed and deposited back in the protocol as an explicit compound action.

On the other hand, SafEth accrues value by the passive appreciation of the underlying LSD tokens backing the protocol. There is no explicit process for claiming or compounding this increase in SafEth token value. The protocol isn't collecting fees from this side of the split.

It is not clear if this is by design or a potential oversight in the implementation.

[L-18] Potential rounding to zero issue in AfEth deposit could cause loss of value

Deposits in the AfEth contract are split based on a configured ratio. One portion of the split goes to SafEth, while the other is deposited in the Votium strategy.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L148-L169

148:     function deposit(uint256 _minout) external payable virtual {
149:         if (pauseDeposit) revert Paused();
150:         uint256 amount = msg.value;
151:         uint256 priceBeforeDeposit = price();
152:         uint256 totalValue;
153: 
154:         AbstractStrategy vStrategy = AbstractStrategy(vEthAddress);
155: 
156:         uint256 sValue = (amount * ratio) / 1e18;
157:         uint256 sMinted = sValue > 0
158:             ? ISafEth(SAF_ETH_ADDRESS).stake{value: sValue}(0)
159:             : 0;
160:         uint256 vValue = (amount * (1e18 - ratio)) / 1e18;
161:         uint256 vMinted = vValue > 0 ? vStrategy.deposit{value: vValue}() : 0;
162:         totalValue +=
163:             (sMinted * ISafEth(SAF_ETH_ADDRESS).approxPrice(true)) +
164:             (vMinted * vStrategy.price());
165:         if (totalValue == 0) revert FailedToDeposit();
166:         uint256 amountToMint = totalValue / priceBeforeDeposit;
167:         if (amountToMint < _minout) revert BelowMinOut();
168:         _mint(msg.sender, amountToMint);
169:     }

The amounts that go into each are calculated in lines 156 and 160. Both sValue and vValue calculations could be rounded down to zero if the numerator is lower than the denominator.

Even with a low ratio value, the amounts lost are negligible, hence the low severity.

For example, given a small ratio of 1% (i.e. 1e16) we have:

amount * ratio < 1e18
amount < 1e18 / ratio
amount < 1e18 / 1e16
amount < 100

Amount should be lower than 100 wei in order to be rounded down to zero.

Non Critical Issues

[NC-1] Remove debug symbols

Remove any code related to debug functionality.

[NC-2] Missing event for important parameter change

Important parameter or configuration changes should trigger an event to allow being tracked off-chain.

Instances (8):

[NC-3] Unused constants

Unreferenced private constants in contracts can be removed.

Instances (2):

[NC-4] Use constants for literal or magic values

Consider defining constants for literal or magic values as it improves readability and prevents duplication of config values.

Instances (9):

Informational Issues

[INFO-1] Consider using the ERC4626 standard

Both AfEth and VotiumStrategy behave like vaults. They take deposits, mint ERC20 tokens in representation of this deposit, and have a withdraw function to recover the assets back.

This is exactly the use case of the ERC4626 standard. Consider using this standard to improve composability.