Basin - erebus's results

Useless modifier in consructor here (even the constructor is empty, just why the modifier there, it makes no sense)

Bad assumptions to calculate the number of block passed here. BLOCK_TIME is an immutable variable which is passed as a parameter to the constructor. However, with future updates to the Ethereum protocol (IDK, reth or other validation algorithm) which could reduce that number would mean that functions dependant on that to work incorrectly like readInstantaneousReserves or _readCumulativeReserves. Do not assume the block rate to remain constant in the future or implement some function that modifies the BLOCK_TIME every T seconds/minutes/whatever

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/f15fe66d57c2f226c232685d16f297e54bcc0939/src/libraries/LibWellConstructor.sol#L71-L81

Vulnerability details

Impact

Writing this

string memory name = LibContractInfo.getSymbol(address(_tokens[0]));
string memory symbol = name;

does not copy the content of name in symbol, what it does is copy the POINTER to the place where the name is stored. Thus, one change in one of them leads to the other pointing to the "updated" variable, breaking the working process of the function and passing wrong arguments to the init function (so the high severity`

Proof of Concept

From Jean Cvllr awesome tutorials about data locations we have

In reality, what happened under the hood is that we create two pointers to memory, named by the variables data and greetings.

When we do data = greetings , we think we are assigning the value cafecafe to the variable data. But we are not assigning anything at all here! We are giving the following instruction to the EVM:

“variable data, I order you to point to the same location in memory that the variable greetings point to!”

(you can read the Yellow paper too but the above is more user-friendly). That means the next code does the same to the two variables because both point to the same place, so one change will affect the other the same way

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");

Because of that, the initFunctionCall = abi.encodeWithSignature("init(string,string)", name, symbol); is broken given the fact that name and symbol are the same.

Tools Used

Manual analysis

Recommended Mitigation Steps

Use local variables instead of memory references/pointers

Assessed type

Error

Do not use full size words for low value variables (more if they are constant). Check here

Change strings in require to custom errors, saving gas on the way. The regex pattern is "\w*"\); and loop through the occurrences

Use unchecked { i++; } in loops where the end is known. The regex pattern is ; ?\w*\++. Save gas because solidity checks integers for over/underflow after 0.8.0 by default, so we can omit that because we know the limit