Beanstalk: The Finale

Summary

From the solidity documentation: https://docs.soliditylang.org/en/v0.8.17/abi-spec.html?highlight=collisions#non-standard-packed-mode > If you use (abi.encodePacked(a, b)) and both a and b are dynamic types, it is easy to craft collisions in the hash value by moving parts of a into b and vice-versa. More specifically, abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c").

Vulnerability Details

The issue is in these lines of code:

https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/main/protocol/contracts/tokens/Fertilizer/Internalizer.sol#L35C5-L37C6

As the solidity docs describe, two or more dynamic types are passed to abi.encodePacked. Moreover, these dynamic values are user-specified function arguments in external functions, meaning anyone can directly specify the value of these arguments when calling the function

Impact

function uri(uint256 _id) public view virtual override returns (string memory) {
return string(abi.encodePacked(_uri, Strings.toString(_id)));
}

Tools Used

Recommendations

Instead of writing functions to accept several arguments that are hashed inside the function, consider rewriting the function to take the hashed value as a function argument directly so that the hashing process happens off-chain. This approach would solve the issue and save gas.