search

code-423n4 / 2022-05-alchemix-findings

5 stars 2 forks source link

No check transferFrom() return value #203

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AutoleverageCurveFactoryethpool.sol#L26 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AutoleverageCurveMetapool.sol#L16

Vulnerability details

Impact

No check transferFrom() return value

Proof of Concept

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AutoleverageCurveFactoryethpool.sol#L26 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AutoleverageCurveMetapool.sol#L16

When calling autoleverage() there is an internal transfer funds from a user to the protocol _transferTokensToSelf(). However, this transfer can silently fail because not all ERC20 tokens revert on transfer failure. Some of them return false. This boolean value is not checked in 2 cases.

Recommended Mitigation Steps

Add checks or use SafeERC20 wrapper.

0xfoobar commented 2 years ago

Sponsor acknowledged

While technically correct, these are specialized helper contracts to help users zap into specific positions using Curve pools and none of the existing setups suffer from this issue. However, it is a better QA design to wrap the token transfers.

0xleastwood commented 2 years ago

Completely agree with sponsor and will be downgrading this to QA and merging with #204.