code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/main/src/Well.sol#L31

Vulnerability details

Impact

As described in cyfrin-basin-audit.pdf Medium Risk 6.2.1: Honest LP provider's liquidity can be drained out directly by calling skim() after he/she added tokens.

Proof of Concept

To fix the 6.2.1 medium bug in cyfrin-basin-audit.pdf, we add uniqueness checks in well's init():

        __ERC20Permit_init(name);
        __ERC20_init(name, symbol);

        IERC20[] memory _tokens = tokens();
        for (uint256 i; i < _tokens.length - 1; ++i) {
            for (uint256 j = i + 1; j < _tokens.length; ++j) {
                if (_tokens[i] == _tokens[j]) {
                    revert DuplicateTokens(_tokens[i]);
                }
            }
        }

However, if the well does not call the function, the tokens can contain two same tokens, and we don't check if our well is initialized in other public/external functions, thus bypassing the fix in cyfrin's report.

Tools Used

Manual Review.

Recommended Mitigation Steps

We can add an initialized modifier and use it for public/external functions:

    modifier initialized() {
        require(bytes(name).length != 0, "Error: Well not initialized.");
        _;
    }

Assessed type

Context

c4-pre-sort commented 1 year ago

141345 marked the issue as low quality report

c4-pre-sort commented 1 year ago

141345 marked the issue as primary issue

141345 commented 1 year ago

for perminsionless contract, users need to check for potential risk, and the init status

maybe QA is more appropriate

alcueca commented 1 year ago

Wells are AMMs. An AMM between a token and itself makes no sense. That the Well can be deployed without being initialized is a valid QA assessment, but the exploit described is invalid.

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)