Basin - josephdara's results

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/libraries/LibContractInfo.sol#L9-L44

Vulnerability details

Impact

The LibContractInfo is used to extract contract information such as name() , decimals() and symbol(). The library also makes room for failure of this contract calls, providing alternatives if the staticcall does not return the required data. However this alternative fails. For the first function, we have

    /**
     * @notice gets the symbol of a given contract
     * @param _contract The contract to get the symbol of
     * @return symbol The symbol of the contract
     * @dev if the contract does not have a symbol function, the first 4 bytes of the address are returned
     */
//@audit-issue Does not return the first 4 bytes of the address IF there is no symbol 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) {
            symbol = abi.decode(data, (string));
        } else {
            assembly {
                mstore(add(symbol, 0x20), shl(224, shr(128, _contract)))
            }
        }
    }

This function Does not return the first four bytes of the address. Instead, It either returns some unreadable characters (Check Foundry POC) or an error (Remix POC). This is a bug as it does not match up the required specs written in the comments, and the code implementation fails. This is also the case for function getName in the Library

 /**
     * @notice gets the name of a given contract
     * @param _contract The contract to get the name of
     * @return name The name of the contract
     * @dev if the contract does not have a name function, the first 8 bytes of the address are returned
     */
    function getName(address _contract) internal view returns (string memory name) {
        (bool success, bytes memory data) = _contract.staticcall(abi.encodeWithSignature("name()"));
        name = new string(8);
        if (success) {
            name = abi.decode(data, (string));
        } else {
            assembly {
                mstore(add(name, 0x20), shl(224, shr(128, _contract)))
            }
        }
    }

This directly impacts the protocol in the LibWellConstructor where these functions are called.

    function encodeWellInitFunctionCall(
        IERC20[] memory _tokens,
        Call memory _wellFunction
    ) public view returns (bytes memory initFunctionCall) {
        string memory name = LibContractInfo.getSymbol(address(_tokens[0]));
        string memory symbol = name;
        for (uint256 i = 1; i < _tokens.length; ++i) {
            name = string.concat(name, ":", LibContractInfo.getSymbol(address(_tokens[i])));
            symbol = string.concat(symbol, LibContractInfo.getSymbol(address(_tokens[i])));
        }
        name = string.concat(name, " ", LibContractInfo.getName(_wellFunction.target), " Well");
        symbol = string.concat(symbol, LibContractInfo.getSymbol(_wellFunction.target), "w");

        // See {Well.init}.
        initFunctionCall = abi.encodeWithSignature("init(string,string)", name, symbol);
    }

The init data is used to initialize a new Well.

Proof of Concept

Here is a runnable POC in foundry with contracts that demo the functionality,

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract Norm{
    function name() external view returns (string memory){
        return "DR Wells";
    }
    function symbol() external view returns (string memory){
        return "DRWs";
    }
}

contract AbNorm{
    function names() external view returns (string memory) {
        return "Abnormal";
    }
    function symbols() external view returns (string memory){
        return "ABRL";
    }
}

library LibContractInfo {
    /**
     * @notice gets the symbol of a given contract
     * @param _contract The contract to get the symbol of
     * @return symbol The symbol of the contract
     * @dev if the contract does not have a symbol function, the first 4 bytes of the address are returned
     */
    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) {
            symbol = abi.decode(data, (string));
        } else {
            assembly {
                mstore(add(symbol, 0x20), shl(224, shr(128, _contract)))
            }
        }
    }

    /**
     * @notice gets the name of a given contract
     * @param _contract The contract to get the name of
     * @return name The name of the contract
     * @dev if the contract does not have a name function, the first 8 bytes of the address are returned
     */
    function getName(address _contract) internal view returns (string memory name) {
        (bool success, bytes memory data) = _contract.staticcall(abi.encodeWithSignature("name()"));
        name = new string(8);
        if (success) {
            name = abi.decode(data, (string));
        } else {
            assembly {
                mstore(add(name, 0x20), shl(224, shr(128, _contract)))
            }
        }
    }

    /**
     * @notice gets the decimals of a given contract
     * @param _contract The contract to get the decimals of
     * @return decimals The decimals of the contract
     * @dev if the contract does not have a decimals function, 18 is returned
     */
    function getDecimals(address _contract) internal view returns (uint8 decimals) {
        (bool success, bytes memory data) = _contract.staticcall(abi.encodeWithSignature("decimals()"));
        decimals = success ? abi.decode(data, (uint8)) : 18; // default to 18 decimals
    }
}
contract Runner is Test{

    Norm normalToken;
    AbNorm abnormalToken;

    function setUp() public {
        normalToken = new Norm();
        abnormalToken = new AbNorm();
    }

    function testNormals() public {
        string memory name_ = LibContractInfo.getName(address(normalToken));
        string memory symbol_ = LibContractInfo.getSymbol(address(normalToken));
        console.log(name_, symbol_);
    }
    function testAbnormals() public {
        string memory name__ = LibContractInfo.getName(address(abnormalToken));
        string memory symbol__ = LibContractInfo.getSymbol(address(abnormalToken));
        console.log(name__, symbol__);
    }

}

The Logs from this test is as follows:

Running 2 tests for test/c4test.t.sol:Runner
[PASS] testAbnormals() (gas: 10128)
Logs:
  .#M� .#M�

[PASS] testNormals() (gas: 11992)
Logs:
  DR Wells DRWs

Test result: ok. 2 passed; 0 failed; finished in 1.92ms

If you remove the foundry import and test the above with this in remix:

contract Runner {

    Norm normalToken;
    AbNorm abnormalToken;

    function setUp() public {
        normalToken = new Norm();
        abnormalToken = new AbNorm();
    }

    function testNormals() public view returns (string memory, string memory){
        string memory name_ = LibContractInfo.getName(address(normalToken));
        string memory symbol_ = LibContractInfo.getSymbol(address(normalToken));
        return (name_, symbol_);
    }
    function testAbnormals() public view returns (string memory, string memory){
        string memory name__ = LibContractInfo.getName(address(abnormalToken));
        string memory symbol__ = LibContractInfo.getSymbol(address(abnormalToken));
        return (name__, symbol__);
    }

}

We get this error when we call testAbnormals() in Remix:

error:
Failed to decode output: null: invalid codepoint at offset 2; missing continuation byte (argument="bytes", value=Uint8Array(0x71e2fdcc00000000), code=INVALID_ARGUMENT, version=strings/5.7.0)

According to the Scope of this contest, libraries/LibContractInfo.sol and libraries/LibWellConstructor.sol are in scope

Tools Used

Remix, Foundry

Recommended Mitigation Steps

I am still improving my assembly skills, but i believe there should be a way to splice some characters off the address in solidity or assembly. If I find a well tested method, I will post it on the discord channel for this contest.

Assessed type

Library

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L22-L43

Vulnerability details

Impact

In the Wells.sol, the open zeppelin's reentrancy guard is imported and inherited but it is not initialized with the init function in the wells contract. Looking at the initialize function of the Reentrancy guard, we see that the initial status is set with the reentrancy guard:

    function __ReentrancyGuard_init() internal onlyInitializing {
        __ReentrancyGuard_init_unchained();
    }

    function __ReentrancyGuard_init_unchained() internal onlyInitializing {
        _status = _NOT_ENTERED;
    }

Although the _status state is modified through other function calls, I believe that this is a bug which should be corrected by the protocol.

Proof of Concept

//@audit-issue reentrancy guard not initialized
    function init(string memory name, string memory symbol) public initializer {
        __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]);
                }
            }
        }
    }

Tools Used

Manual review

Recommended Mitigation Steps

I suggest that the __ReentrancyGuard_init() function be called inside the Well.init() so as to initialize the ReentrancyGuard as well.

Assessed type

Other