Basin - 0xkazim's results

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/libraries/LibWellConstructor.sol#L73-L79 https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/libraries/LibContractInfo.sol#L16-L27

Vulnerability details

Impact

The getSymbol() function, found in the LibContractInfo.sol contract and can cause a revert when it called in any functions. This is because the getSymbol This could make the contract or function that calling this function reverts, because the getSymbol() function will return string if the call to the contract is true(the contract have symbol() function) and this call can run the if(success = true) but then revert in returning the symbol in string.

note that these function are only called in LibWellConstructor function but still valid because the LibContractInfo is in scope and this libs could be used in future because the team want to audit it.

Proof of Concept

The root cause of the issue is that the getSymbol() function assumes the return type of any ERC20 token to be a string. If the return value is not a string, abi.decode() will revert, and this could happen because getSymbol make a call to the _contract and if this contract contain a function called symbol() then the if (success) will be run but the next line will revert because the Symbol() function in the _contract did not return symbol as a string but it returns it as bytes32

the getSymbol() function:

function getSymbol(address _contract) internal view returns (string memory symbol) {
        (bool success, bytes memory data) = _contract.staticcall(abi.encodeWithSignature("symbol()"));
        symbol = new string(4);
        if (success) {
            //@audit
            symbol = abi.decode(data, (string));
        } else {
            assembly {
                mstore(add(symbol, 0x20), shl(224, shr(128, _contract)))
            }
        }
    }

this function only handle if the function symbol not exist in the _contract and did not handle the case of returning symbol as bytes32

as you can see the function will run the success = true when the contract have symbol() function without handle the return of symbol if its string or bytes.

Because this is known to cause issues with tokens that don't fully follow the ERC20 spec, the safeSymbol() function in the BoringCrypto library has a fix for this. The BoringCrypto safeSymbol() function is similar to the one in basin LibContractInfo but it has a returnDataToString() function that handles the case of a bytes32 return value for a token symbol:

https://github.com/boringcrypto/BoringSolidity/blob/ccb743d4c3363ca37491b87c6c9b24b1f5fa25dc/contracts/libraries/BoringERC20.sol#L15-L39

if this function called in any of the basin contracts in future may lead to dos because of unhandled returns of bytes rather than string

Tools Used

manual review

Recommended Mitigation Steps

Use the BoringCrypto safeSymbol() function code to handle the case of a bytes32 return value:

https://github.com/boringcrypto/BoringSolidity/blob/ccb743d4c3363ca37491b87c6c9b24b1f5fa25dc/contracts/libraries/BoringERC20.sol#L15-L47

Assessed type

Other