Platform: Code4rena
Start Date: 01/12/2022
Pot Size: $60,500 USDC
Total HM: 8
Participants: 27
Period: 11 days
Judge: kirk-baird
Total Solo HM: 6
Id: 187
League: ETH
Rank: 13/27
Findings: 2
Award: $119.07
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0xSmartContract
Also found by: 0x1f8b, 0xDecorativePineapple, Chom, Deivitto, IllIllI, Jeiwan, Josiah, Mukund, RaymondFam, Rolezn, ajtra, cccz, chrisdior4, csanuragjain, hansfriese, ladboy233, minhquanym, pedr02b2, peritoflores, rvierdiiev, sakshamguruji, sces60107
59.8382 USDC - $59.84
The following contracts and functions, allow owners to interact with functions such as:
Position.sol#setMetadata()
PositionMetadata.sol#setBaseURI()
Given that Position.sol and PositionMetadata.sol are derived from Ownable, the ownership management of this contract defaults to Ownable ’s transferOwnership() and renounceOwnership() methods which are not overridden here.
Such critical address transfer/renouncing in one-step is very risky because it is irrecoverable from any mistakes
Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately.
When noticed, due to a failing onlyOwner() function call, it will force the redeployment of these contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in the protocol and incur a significant reputational damage.
Ownable
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Position.sol#L12
contract Position is ERC721, ERC721Enumerable, Ownable, IPosition {https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/PositionMetadata.sol#L8 contract PositionMetadata is IPositionMetadata, Ownable {
See similar High Risk severity finding from Trail-of-Bits Audit of Hermez. https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: https://github.com/Uniswap/v3-core/blob/main/audits/tob/audit.pdf
Recommend overriding the inherited methods to null functions and use separate functions for a two-step address change:
pendingOwnerpendingOwner address claims the pending ownership change.This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.
Also, consider adding a time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.
immutable variablesZero address should be checked for state variables, immutable variables. A zero address can lead into problems.
constructor(IFactory _factory, IPosition _position, IWETH9 _WETH9) { factory = _factory; position = _position; WETH9 = _WETH9; }
Check zero address before assigning or using it
Some lines use // x and some use //x. The instances below point out the usages that don't follow the common way, within each file
uint256 public genericDeclaration;//generic comment without space
But following the style of the other comments would be:
uint256 public genericDeclaration; // generic comment with space
// // The multiplication in the next line has the same exact purpose // as the one above. https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/libraries/BytesLib.sol#L44
//commenthttps://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L78 //to the current bin or an absolute position
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L100 //defined in Pool.sol
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L147 //position of the liquidity
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L149 //caller can transfer tokens
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L167 //mergeId is the currrent active bin.
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L170 //zero to recurse until the active bin is found.
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L176 //is false or the output if exactOutput is true
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L179 //exact output amount (true)
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L181 //indicates no limit. Limit is only engaged for exactOutput=false. If the
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L182 //limit is reached only part of the input amount will be swapped and the
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L183 //callback will only require that amount of the swap to be paid.
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L185 //caller can transfer tokens
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPoolAdmin.sol#L7 //or B protocol fee (2 or 3)
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/Router.sol#L21 //computed amount in for an exact output swap / can never actually be this
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/libraries/BytesLib.sol#L56 //update free-memory pointer
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/libraries/BytesLib.sol#L57 //allocating the array padded to 32 bytes like the compiler does now
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/libraries/BytesLib.sol#L60 //if we want a zero-length slice let's just return a zero-length array
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/libraries/BytesLib.sol#L63 //zero out the 32 bytes slice we are about to return
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/libraries/BytesLib.sol#L64 //we need to do it because Solidity does not garbage collect
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/libraries/Multicall.sol#L17 // Next 5 lines from https://ethereum.stackexchange.com/a/83577
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/interfaces/IRouter.sol#L48 //another along the specified path
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/interfaces/IRouter.sol#L50
//as ExactInputParams in calldata
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/interfaces/IRouter.sol#L80 //another along the specified path (reversed)
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/interfaces/IRouter.sol#L82
//as ExactOutputParams in calldata
Long lines should be wrapped to conform with Solidity Style guidelines.
Lines that exceed the 120 character length suggested by the Solidity Style guidelines. Reference: https://docs.soliditylang.org/en/v0.8.16/style-guide.html#maximum-line-length
Reduce line length to less than 120 at least to improve maintainability and readability of the code
#0 - c4-judge
2022-12-15T10:47:44Z
kirk-baird marked the issue as grade-b
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0xSmartContract, Deivitto, Mukund, RaymondFam, Rolezn, ajtra, c3phas, sakshamguruji, saneryee
59.2291 USDC - $59.23
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation.
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/PoolInspector.sol#L65 try pool.swap(address(this), amount, tokenAIn, exactOutput, 0, abi.encode(exactOutput)) {} catch Error(string memory _data) {
Remove empty block, revert or emit something.
Unchecked operations as the ++i on for loops are cheaper than checked one.
In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline..
The code would go from:
for (uint256 i; i < numIterations; i++) { // ... }
to
for (uint256 i; i < numIterations;) { // ... unchecked { ++i; } } The risk of overflow is inexistent for a uint256 here.
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L127 for (uint256 i; i < params.length; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L180 for (uint256 i; i < binIds.length; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L193 for (uint256 i; i < params.length; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L224 for (uint256 i; i < params.length; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L380 for (uint256 j; j < 2; j++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L389 for (uint256 i; i <= moveData.binCounter; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L414 for (uint256 i; i < moveData.mergeBinCounter; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L523 for (uint256 i; i < NUMBER_OF_KINDS; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L540 for (uint256 i; i < NUMBER_OF_KINDS; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L653 for (uint256 i; i < swapData.counter; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/PoolInspector.sol#L30 for (uint128 i = startBinIndex; i < binCounter; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/PoolInspector.sol#L80 for (uint8 i = 0; i < NUMBER_OF_KINDS; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/PoolInspector.sol#L101 for (uint256 i; i < bins.length; i++) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/libraries/Multicall.sol#L13 for (uint256 i = 0; i < data.length; i++) {
Add unchecked ++i at the end of all the for loop where it's not expected to overflow and remove them from the for header
Gas efficiency can be achieved by tightly packing the struct. Struct variables are stored in 32 bytes each so you can group smaller types to occupy less storage.
You can read more here: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html or in the official documentation: https://docs.soliditylang.org/en/v0.4.21/miscellaneous.html
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/PoolInspector.sol#L14 struct BinInfo {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/libraries/Delta.sol#L5 struct Instance {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/interfaces/IPool.sol#L34 struct BinDelta {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/Pool.sol#L363 struct MoveData {
Order structs to reduce gas usage.
abi.encode() is less gas efficient than abi.encodePacked()In general, abi.encodePacked is more gas-efficient.
Changing the abi.encode function to abi.encodePacked can save gas since the abi.encode function pads extra null bytes at the end of the call data, which is unnecessary. Also, in general, abi.encodePacked is more gas-efficient.
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/PoolInspector.sol#L58 revert(string(abi.encode(amountIn)));
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/PoolInspector.sol#L60 revert(string(abi.encode(amountOut)));
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/maverick-v1/contracts/models/PoolInspector.sol#L65 try pool.swap(address(this), amount, tokenAIn, exactOutput, 0, abi.encode(exactOutput)) {} catch Error(string memory _data) {
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/Router.sol#L128 (, amountOut) = pool.swap(recipient, amountIn, tokenAIn, false, sqrtPriceLimitD18, abi.encode(data));
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/Router.sol#L176 (amountIn, amountOutReceived) = pool.swap(recipient, amountOut, tokenAIn, true, 0, abi.encode(data));
https://github.com/code-423n4/2022-12-Stealth-Project/blob/2c798a70333c83102affd19c4346412be4bf859e/router-v1/contracts/Router.sol#L232 (tokenAAmount, tokenBAmount, binDeltas) = pool.addLiquidity(tokenId, params, abi.encode(data));
Consider changing abi.encode to abi.encodePacked
#0 - c4-judge
2022-12-15T10:49:01Z
kirk-baird marked the issue as grade-b