code-423n4 / 2023-11-shellprotocol-findings

7 stars 7 forks source link

Upgraded Q -> 2 from #186 [1702725969627] #340

Closed c4-judge closed 8 months ago

c4-judge commented 8 months ago

Judge has assessed an item in Issue #186 as 2 risk. The relevant finding follows:

L-02 Dust from >18 decimal tokens will collect in Curve pool adapters

When swapping in Curve pools the amounts are converted back and forth between whatever the token decimal is and Oceans 18 decimal accounting. The issue is that if the token converted has more than 18 decimals the left overs are truncated:

Curve2PoolAdapter::primitiveOutputAmount:

File: src/adapters/Curve2PoolAdapter.sol

173:        outputAmount = _convertDecimals(decimals[outputToken], NORMALIZED_DECIMALS, rawOutputAmount);

If decimals[outputToken] is greater than NORMALIZED_DECIMALS, that will cause truncation in OceanAdapter::_convertDecimals:

File: src/adapters/OceanAdapter.sol

154:        } else {
155:            // Decimal shift right (remove precision) -> truncation
156:            uint256 shift = 10 ** (uint256(decimalsFrom - decimalsTo));
157:            convertedAmount = amountToConvert / shift;
158:        }

Then the amount is, again, scaled up in Ocean::_convertDecimals:

File: src/ocean/Ocean.sol

1136:        } else if (decimalsFrom < decimalsTo) {
1137:            // Decimal shift left (add precision)
1138:            uint256 shift = 10 ** (uint256(decimalsTo - decimalsFrom));
1139:            convertedAmount = amountToConvert * shift;
1140:            truncatedAmount = 0;
1141:        } else {

Imagine a 24 decimal token. The swap resultus in an amount 123_456_789_012_345_678_901_234. This will be converted to Ocean 18 decimals, 123_456_789_012_345_678 in Ocean accounting. Then when the wrapping of the ERC20 happens, it is scaled up to 24 decimals again. Here, however without the last information: 123_456_789_012_345_678_000_000. Thus, 901_234 of the token will be left in the Curve pool adapter contract.

Recommendation

Consider adding a public sweep option to the Curve pool adapters, could be locked to owner or public.

Non critical / Informational

c4-judge commented 8 months ago

0xA5DF marked the issue as duplicate of #252

c4-judge commented 8 months ago

0xA5DF marked the issue as satisfactory

c4-judge commented 8 months ago

This auto-generated issue was withdrawn by 0xA5DF

c4-judge commented 8 months ago

0xA5DF marked the issue as grade-c

0xA5DF commented 8 months ago

Moved to #186