Alchemix contest - GimelSec's results

Summary

We list 1 low-critical findings

• (low) reApprove() doesn’t check alcx.approve()’s return value

(low) reApprove() doesn’t check alcx.approve()’s return value

Impact

In gALCX.sol, it is better to check alcx.approve()’s return value in reApprove()

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Check return value of alcx.approve

validBridgeToken modifier is duplicated in exchangeOldForCanonical() and exchangeCanonicalForOld

Proof of Concept

In exchangeOldForCanonical() and exchangeCanonicalForOld(), validBridgeToken modifier is used to check bridgeTokenEnabled[tokenAddress]

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/CrossChainCanonicalBase.sol#L33-L38

    modifier validBridgeToken(address tokenAddress) {
        if (!bridgeTokenEnabled[tokenAddress]) {
            revert IllegalState();
        }
        _;
    }

However in both functions, bridgeTokenEnabled[tokenAddress] is check again.

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/CrossChainCanonicalBase.sol#L91

    function exchangeOldForCanonical(address bridgeTokenAddress, uint256 tokenAmount) external nonReentrant validBridgeToken(bridgeTokenAddress) returns (uint256 canonicalTokensOut) {
        …
        if (!bridgeTokenEnabled[bridgeTokenAddress]) {
            revert IllegalState();
        }
        …
    }

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/CrossChainCanonicalBase.sol#L114

    function exchangeCanonicalForOld(address bridgeTokenAddress, uint256 tokenAmount) external nonReentrant validBridgeToken(bridgeTokenAddress) returns (uint256 bridgeTokensOut) {
        …
        if (!bridgeTokenEnabled[bridgeTokenAddress]) {
            revert IllegalState();
        }
        …
    }

Recommendation

It’s duplicated. One of the checks should be removed to save gas.

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

ThreePoolAssetManager.sol
250:        for (uint256 i = 0; i < NUM_STABLE_COINS; i++) {
254:        for (uint256 i = 0; i < NUM_META_COINS; i++) {
353:        for (uint256 i = 0; i < 256; i++) {
773:        for (uint256 i = 0; i < NUM_STABLE_COINS; i++) {
902:        for (uint256 i = 0; i < NUM_META_COINS; i++) {

test/TransmuterBufferMock.sol
15:    for (uint256 i = 0; i < _underlyingTokens.length; i++) {

EthAssetManager.sol
214:        for (uint256 i = 0; i < NUM_META_COINS; i++) {
567:        for (uint256 i = 0; i < NUM_META_COINS; i++) {

CrossChainCanonicalBase.sol
57:        for (uint256 i = 0; i < _bridgeTokens.length; i++){
141:        for (uint i = 0; i < bridgeTokensArray.length; i++){

TransmuterBuffer.sol
172:        for (uint256 i = 0; i < _yieldTokens[underlyingToken].length; i++) {
186:        for (uint256 i = 0; i < tokens.length; i++) {
235:            for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
242:        for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
272:        for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
382:        for (uint256 j = 0; j < registeredUnderlyings.length; j++) {
387:        for (uint256 i = 0; i < numYTokens; i++) {
479:        for (uint256 j = 0; j < weighting.tokens.length; j++) {

AlchemistV2.sol
990:        for (uint256 i = 0; i < depositedTokens.values.length; i++) {
1282:        for (uint256 i = 0; i < depositedTokens.values.length; i++) {
1355:        for (uint256 i = 0; i < depositedTokens.values.length; i++) {
1461:        for (uint256 i = 0; i < depositedTokens.values.length; i++) {
1524:        for (uint256 i = 0; i < depositedTokens.values.length; i++) {

StakingPools.sol
188:    for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) {
363:    for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) {

base/Multicall.sol
14:        for (uint256 i = 0; i < data.length; i++) {

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}