Alchemix contest - 0xsomeone's results

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2Base.sol#L189-L191

Vulnerability details

ATB-01M: Inexplicable Capability of Self-Limit Reduction

Description

The mint function is meant to prevent arbitrary amount mints via the totalMinted variable being guaranteed to be less than mintCeiling as set by the administrator of the system. However, the lowerHasMinted function permits a whitelisted member to lower their own mints thereby effectively bypassing the mintCeiling check and rendering it redundant.

Impact

The whitelisted parties can completely bypass the mintCeiling limits set by the administrator thereby rendering minting entirely up to the discretion of the whitelisted parties.

Solution (Recommended Mitigation Steps)

We advise alternative access control to be imposed by lowerHasMinted so that the restriction structure for mints makes sense in the system.

PoC

Issue is immediately deducible by inspecting the relevant lines referenced in the issue.

Tools

Manual inspection of the codebase.

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/gALCX.sol#L69-L81

Vulnerability details

GAX-01M: Exchange Rate Race Condition

Description

The gALCX contains a race condition whereby whenever the contract has no stakes (such as when the contract is first deployed) the exchangeRate can be artificially inflated via direct ALCX transfers. In detail, a user can make a small ALCX deposit, perform a direct transfer of ALCX to the contract, redeem the ALCX and repeat to artifically inflate the exchangeRate to a potentially significant degree affecting truncations.

Impact

The exchangeRate can be trivially inflated rendering the contract usable only by users making large deposits (due to truncation) or entirely unusable if the rate is inflated to a significant degree surpassing a decent percentage of the total supply of the ALCX asset. We would like to note that since inflation is possible via the usage of flash-loans, the size of exchangeRate can be easily exarcebated to significant proportions.

Solution (Recommended Mitigation Steps)

We advise the bumpExchangeRate function to contain an alternative execution path that checks if the totalSupply is zero and in such a case simply resets the exchangeRate to 1e18 to ensure that the contract behaves properly under all circumstances. Alternatively, if the exchangeRate is envisioned to continuously increase perpetually (a trait we advise against), the code should evaluate the delta between balanceOf invocations "sandwhiching" the claim operation similarly to how the various adapters of the protocol operate.

PoC

Issue is deducible by inspecting the relevant lines referenced in the issue and making note of the balanceOf invocation that does not care about whether the funds came from the claim operation.

Tools

Manual inspection of the codebase.

Alchemix Audit Report (QA)

ATV-01L: Inexistent Sanitization of Flash Loan Fee

Description

The mint fee is meant to represent a percentage of the BPS variable, however, no upper cap is imposed on it.

Solution

We advise it to be at most equivalent to BPS (in reality it will be significantly less) to ensure that the flash loan fee is service-able.

ATB-01L: Inexistent Sanitization of Flash Loan Fee

Description

The mint fee is meant to represent a percentage of the BPS variable, however, no upper cap is imposed on it.

Solution

We advise it to be at most equivalent to BPS (in reality it will be significantly less) to ensure that the flash loan fee is service-able.

CCB-01L: Inexistent Sanitization of Duplicate Entries

Description

The constructor of the contract does not sanitize against duplicate entries in the _bridgeTokens in contrast to addBridgeToken.

Solution

We advise such sanitization to be imposed to prevent a misconfigured bridgeTokensArray.

CCB-02L: Improper Maintenance of Mapping Entries

Description

The bridgeTokens mapping is improperly maintained as it is solely adjusted during the constructor and not during the addBridgeToken.

Solution

We advise the entry to be properly updated when a bridge token is added to ensure the data source is in sync.

CCB-03L: Inexistent Sanitization of Swap Fees

Description

The swap fees must be ensured to be at most equivalent to FEE_PRECISION as otherwise all swapping operations will result in underflows.

Solution

We advise these limits to be imposed on the function via corresponding if-revert checks.

CCB-04N: Inconsistent Programming Paradigm

Description

The linked functions are prefixed with the toggle keyword, however, one acts as a setter function while the other actually toggles the key's value.

Solution

We advise behaviour to be streamlined across the codebase to avoid ambiguous behaviour.

GAX-01L: Non-Standard Usage of EIP-20 Functions

Description

The linked EIP-20 functions are mandated to yield a bool variable for validation of the call's execution.

Solution

While callers should not assume that a bool is not returned, some tokens such as Tether are non-standard and yield no bool variable thereby causing requirechecks to fail. We advise the localTokenUtils` library to be utilized instead as it already exists in the codebase and guarantees safe execution regardless of the underlying token implementation.

GAX-02L: Permanent Lock of Funds by Mathematical Truncation

Description

The stake and unstake functions perform conversions between two assets via multiplications and divisions, however, they do not account for truncations thereby causing funds to be permanently lost during either transaction. Given that truncations can and will occur on almost each transaction, the lost value can significantly compound.

Solution

We advise the functions to instead consume an amount or gAmount that is wholly divisible by the exchange. This can be achieved via the usage of a modulo (%) operation and subtraction of the indivisible remainder from the original stake / unstake amount.

SCT-01N: Code Style Standardization

Description

The linked literals depict upper and lower limits respectively of a specific numeric type (int256 and uint256 respectively).

Solution

We advise the type specifier to be utilized along with the max and min members to standardize the code style of the code (i.e 2**255 is replaced by type(int256).max and 0 is replaced by type(uint256).min without affecting gas cost).

Alchemix Audit Report (Gas Optimizations)

CCB-01G: Redundantly Expensive Loop Validation

Description

The loop validation performed between lines L141-L145 is entirely redundant as the bridgeTokens mapping can and should be utilized instead.

Solution

We advise the bridgeTokens mapping to be utilized for this type of validation greatly reducing the gas cost of the function.

CCB-02G: Inefficient Loop Limit Evaluation

Description

The linked for loop will dynamically evaluate its limit on each iteration as the compiler cannot deduce whether the internal statements of the loop affect the length member of the bridgeTokensArray (i.e. push to / pop from it).

Solution

We advise the length to be cached to a local variable that is consequently utilized for validating the limit thereby optimizing the function's gas cost.

GAX-01G: Variable Mutability Optimization

Description

The linked variable is assigned to only once during its declaration.

Solution

We advise it to be set as constant significantly optimizing its read access gas cost.

GAX-02G: Redundant Local Variable

Description

The local success variable is redundant as it is not utilized in the code. Additionally, should the ALCX token not return a bool for the approve function execution the call would fail.

Solution

While the code is safe as the ALCX token does yield a bool variable, we still advise the local variable and corresponding assignment to be omitted to optimize the code's execution cost.

LMH-01G: Redundant Overflow / Underflow Checks

Description

The codebase is compiled with a version greater than 0.8.X and has built-in safe arithmetics toggled on. As a result, explicit overflow / underflow checks are redundant.

Solution

We advise them to be omitted in favor of reduced gas costs. Additionally, we advise the pragma statement of the contract to be updated.

RPL-01G: Redundant Dynamic Evaluation of getAddress Key

Description

The result of keccak256(abi.encodePacked("contract.address", "rocketTokenRETH")) is deterministic and equivalent to the literal 0xe3744443225bff7cc22028be036b80de58057d65a3fdca0a3df329f525e31ccc.

Solution

We advise the statements to be replaced by the literal optimizing the code's gas cost.

EAM-01G: Variable Mutability Optimization

Description

The linked variable is assigned to only once during the contract's constructor execution.

Solution

We advise it to be set as immutable significantly optimizing its read access gas cost.