Skip to content

Commit c5cc553

Browse files
chrisethchriseth
authored
Merge pull request #12850 from ethereum/dataLocationInInheritance
Properly check data location in inheritance.
2 parents bef348a + f427247 commit c5cc553

20 files changed

+222
-13
lines changed

‎Changelog.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
Important Bugfixes:
44
* ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against ``calldatasize()`` in all cases.
5+
* Override Checker: Allow changing data location for parameters only when overriding external functions.
56

67

78
Compiler Features:

‎docs/bugs.json

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,15 @@
11
[
2+
{
3+
"uid": "SOL-2022-3",
4+
"name": "DataLocationChangeInInternalOverride",
5+
"summary": "It was possible to change the data location of the parameters or return variables from ``calldata`` to ``memory`` and vice-versa while overriding internal and public functions. This caused invalid code to be generated when calling such a function internally through virtual function calls.",
6+
"description": "When calling external functions, it is irrelevant if the data location of the parameters is ``calldata`` or ``memory``, the encoding of the data does not change. Because of that, changing the data location when overriding external functions is allowed. The compiler incorrectly also allowed a change in the data location for overriding public and internal functions. Since public functions can be called internally as well as externally, this causes invalid code to be generated when such an incorrectly overridden function is called internally through the base contract. The caller provides a memory pointer, but the called function interprets it as a calldata pointer or vice-versa.",
7+
"link": "https://blog.soliditylang.org/2022/05/17/data-location-inheritance-bug/",
8+
"introduced": "0.6.9",
9+
"fixed": "0.8.14",
10+
"severity": "very low"
11+
12+
},
213
{
314
"uid": "SOL-2022-2",
415
"name": "NestedCallataArrayAbiReencodingSizeValidation",

‎docs/bugs_by_version.json

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1345,6 +1345,7 @@
13451345
},
13461346
"0.6.10": {
13471347
"bugs": [
1348+
"DataLocationChangeInInternalOverride",
13481349
"NestedCallataArrayAbiReencodingSizeValidation",
13491350
"SignedImmutables",
13501351
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1356,6 +1357,7 @@
13561357
},
13571358
"0.6.11": {
13581359
"bugs": [
1360+
"DataLocationChangeInInternalOverride",
13591361
"NestedCallataArrayAbiReencodingSizeValidation",
13601362
"SignedImmutables",
13611363
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1367,6 +1369,7 @@
13671369
},
13681370
"0.6.12": {
13691371
"bugs": [
1372+
"DataLocationChangeInInternalOverride",
13701373
"NestedCallataArrayAbiReencodingSizeValidation",
13711374
"SignedImmutables",
13721375
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1477,6 +1480,7 @@
14771480
},
14781481
"0.6.9": {
14791482
"bugs": [
1483+
"DataLocationChangeInInternalOverride",
14801484
"NestedCallataArrayAbiReencodingSizeValidation",
14811485
"SignedImmutables",
14821486
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1489,6 +1493,7 @@
14891493
},
14901494
"0.7.0": {
14911495
"bugs": [
1496+
"DataLocationChangeInInternalOverride",
14921497
"NestedCallataArrayAbiReencodingSizeValidation",
14931498
"SignedImmutables",
14941499
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1500,6 +1505,7 @@
15001505
},
15011506
"0.7.1": {
15021507
"bugs": [
1508+
"DataLocationChangeInInternalOverride",
15031509
"NestedCallataArrayAbiReencodingSizeValidation",
15041510
"SignedImmutables",
15051511
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1512,6 +1518,7 @@
15121518
},
15131519
"0.7.2": {
15141520
"bugs": [
1521+
"DataLocationChangeInInternalOverride",
15151522
"NestedCallataArrayAbiReencodingSizeValidation",
15161523
"SignedImmutables",
15171524
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1523,6 +1530,7 @@
15231530
},
15241531
"0.7.3": {
15251532
"bugs": [
1533+
"DataLocationChangeInInternalOverride",
15261534
"NestedCallataArrayAbiReencodingSizeValidation",
15271535
"SignedImmutables",
15281536
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1533,6 +1541,7 @@
15331541
},
15341542
"0.7.4": {
15351543
"bugs": [
1544+
"DataLocationChangeInInternalOverride",
15361545
"NestedCallataArrayAbiReencodingSizeValidation",
15371546
"SignedImmutables",
15381547
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1542,6 +1551,7 @@
15421551
},
15431552
"0.7.5": {
15441553
"bugs": [
1554+
"DataLocationChangeInInternalOverride",
15451555
"NestedCallataArrayAbiReencodingSizeValidation",
15461556
"SignedImmutables",
15471557
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1551,6 +1561,7 @@
15511561
},
15521562
"0.7.6": {
15531563
"bugs": [
1564+
"DataLocationChangeInInternalOverride",
15541565
"NestedCallataArrayAbiReencodingSizeValidation",
15551566
"SignedImmutables",
15561567
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1560,6 +1571,7 @@
15601571
},
15611572
"0.8.0": {
15621573
"bugs": [
1574+
"DataLocationChangeInInternalOverride",
15631575
"NestedCallataArrayAbiReencodingSizeValidation",
15641576
"SignedImmutables",
15651577
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1569,6 +1581,7 @@
15691581
},
15701582
"0.8.1": {
15711583
"bugs": [
1584+
"DataLocationChangeInInternalOverride",
15721585
"NestedCallataArrayAbiReencodingSizeValidation",
15731586
"SignedImmutables",
15741587
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1578,32 +1591,37 @@
15781591
},
15791592
"0.8.10": {
15801593
"bugs": [
1594+
"DataLocationChangeInInternalOverride",
15811595
"NestedCallataArrayAbiReencodingSizeValidation"
15821596
],
15831597
"released": "2021-11-09"
15841598
},
15851599
"0.8.11": {
15861600
"bugs": [
1601+
"DataLocationChangeInInternalOverride",
15871602
"NestedCallataArrayAbiReencodingSizeValidation",
15881603
"AbiEncodeCallLiteralAsFixedBytesBug"
15891604
],
15901605
"released": "2021-12-20"
15911606
},
15921607
"0.8.12": {
15931608
"bugs": [
1609+
"DataLocationChangeInInternalOverride",
15941610
"NestedCallataArrayAbiReencodingSizeValidation",
15951611
"AbiEncodeCallLiteralAsFixedBytesBug"
15961612
],
15971613
"released": "2022-02-16"
15981614
},
15991615
"0.8.13": {
16001616
"bugs": [
1617+
"DataLocationChangeInInternalOverride",
16011618
"NestedCallataArrayAbiReencodingSizeValidation"
16021619
],
16031620
"released": "2022-03-16"
16041621
},
16051622
"0.8.2": {
16061623
"bugs": [
1624+
"DataLocationChangeInInternalOverride",
16071625
"NestedCallataArrayAbiReencodingSizeValidation",
16081626
"SignedImmutables",
16091627
"ABIDecodeTwoDimensionalArrayMemory",
@@ -1613,6 +1631,7 @@
16131631
},
16141632
"0.8.3": {
16151633
"bugs": [
1634+
"DataLocationChangeInInternalOverride",
16161635
"NestedCallataArrayAbiReencodingSizeValidation",
16171636
"SignedImmutables",
16181637
"ABIDecodeTwoDimensionalArrayMemory"
@@ -1621,34 +1640,39 @@
16211640
},
16221641
"0.8.4": {
16231642
"bugs": [
1643+
"DataLocationChangeInInternalOverride",
16241644
"NestedCallataArrayAbiReencodingSizeValidation",
16251645
"SignedImmutables"
16261646
],
16271647
"released": "2021-04-21"
16281648
},
16291649
"0.8.5": {
16301650
"bugs": [
1651+
"DataLocationChangeInInternalOverride",
16311652
"NestedCallataArrayAbiReencodingSizeValidation",
16321653
"SignedImmutables"
16331654
],
16341655
"released": "2021-06-10"
16351656
},
16361657
"0.8.6": {
16371658
"bugs": [
1659+
"DataLocationChangeInInternalOverride",
16381660
"NestedCallataArrayAbiReencodingSizeValidation",
16391661
"SignedImmutables"
16401662
],
16411663
"released": "2021-06-22"
16421664
},
16431665
"0.8.7": {
16441666
"bugs": [
1667+
"DataLocationChangeInInternalOverride",
16451668
"NestedCallataArrayAbiReencodingSizeValidation",
16461669
"SignedImmutables"
16471670
],
16481671
"released": "2021-08-11"
16491672
},
16501673
"0.8.8": {
16511674
"bugs": [
1675+
"DataLocationChangeInInternalOverride",
16521676
"NestedCallataArrayAbiReencodingSizeValidation",
16531677
"UserDefinedValueTypesBug",
16541678
"SignedImmutables"
@@ -1657,6 +1681,7 @@
16571681
},
16581682
"0.8.9": {
16591683
"bugs": [
1684+
"DataLocationChangeInInternalOverride",
16601685
"NestedCallataArrayAbiReencodingSizeValidation"
16611686
],
16621687
"released": "2021-09-29"

‎libsolidity/analysis/ContractLevelChecker.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ namespace
3939
{
4040

4141
template <class T, class B>
42-
bool hasEqualParameters(T const& _a, B const& _b)
42+
bool hasEqualExternalCallableParameters(T const& _a, B const& _b)
4343
{
4444
return FunctionType(_a).asExternallyCallableFunction(false)->hasEqualParameterTypes(
4545
*FunctionType(_b).asExternallyCallableFunction(false)
@@ -204,7 +204,7 @@ void ContractLevelChecker::findDuplicateDefinitions(map<string, vector<T>> const
204204
SecondarySourceLocation ssl;
205205

206206
for (size_t j = i + 1; j < overloads.size(); ++j)
207-
if (hasEqualParameters(*overloads[i], *overloads[j]))
207+
if (hasEqualExternalCallableParameters(*overloads[i], *overloads[j]))
208208
{
209209
solAssert(
210210
(

‎libsolidity/analysis/OverrideChecker.cpp

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ Token OverrideProxy::functionKind() const
313313
}, m_item);
314314
}
315315

316-
FunctionType const* OverrideProxy::functionType() const
316+
FunctionType const* OverrideProxy::externalFunctionType() const
317317
{
318318
return std::visit(GenericVisitor{
319319
[&](FunctionDefinition const* _item) { return FunctionType(*_item).asExternallyCallableFunction(false); },
@@ -322,6 +322,15 @@ FunctionType const* OverrideProxy::functionType() const
322322
}, m_item);
323323
}
324324

325+
FunctionType const* OverrideProxy::originalFunctionType() const
326+
{
327+
return std::visit(GenericVisitor{
328+
[&](FunctionDefinition const* _item) { return TypeProvider::function(*_item); },
329+
[&](VariableDeclaration const*) -> FunctionType const* { solAssert(false, "Requested specific function type of variable."); return nullptr; },
330+
[&](ModifierDefinition const*) -> FunctionType const* { solAssert(false, "Requested specific function type of modifier."); return nullptr; }
331+
}, m_item);
332+
}
333+
325334
ModifierType const* OverrideProxy::modifierType() const
326335
{
327336
return std::visit(GenericVisitor{
@@ -413,7 +422,7 @@ OverrideProxy::OverrideComparator const& OverrideProxy::overrideComparator() con
413422
[&](FunctionDefinition const* _function)
414423
{
415424
vector<string> paramTypes;
416-
for (Type const* t: functionType()->parameterTypes())
425+
for (Type const* t: externalFunctionType()->parameterTypes())
417426
paramTypes.emplace_back(t->richIdentifier());
418427
return OverrideComparator{
419428
_function->name(),
@@ -424,7 +433,7 @@ OverrideProxy::OverrideComparator const& OverrideProxy::overrideComparator() con
424433
[&](VariableDeclaration const* _var)
425434
{
426435
vector<string> paramTypes;
427-
for (Type const* t: functionType()->parameterTypes())
436+
for (Type const* t: externalFunctionType()->parameterTypes())
428437
paramTypes.emplace_back(t->richIdentifier());
429438
return OverrideComparator{
430439
_var->name(),
@@ -589,21 +598,54 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr
589598

590599
if (_super.isFunction())
591600
{
592-
FunctionType const* functionType = _overriding.functionType();
593-
FunctionType const* superType = _super.functionType();
601+
FunctionType const* functionType = _overriding.externalFunctionType();
602+
FunctionType const* superType = _super.externalFunctionType();
594603

604+
bool returnTypesDifferAlready = false;
595605
if (_overriding.functionKind() != Token::Fallback)
596606
{
597607
solAssert(functionType->hasEqualParameterTypes(*superType), "Override doesn't have equal parameters!");
598608

599609
if (!functionType->hasEqualReturnTypes(*superType))
610+
{
611+
returnTypesDifferAlready = true;
600612
overrideError(
601613
_overriding,
602614
_super,
603615
4822_error,
604616
"Overriding " + _overriding.astNodeName() + " return types differ.",
605617
"Overridden " + _overriding.astNodeName() + " is here:"
606618
);
619+
}
620+
}
621+
622+
// The override proxy considers calldata and memory the same data location.
623+
// Here we do a more specific check:
624+
// Data locations of parameters and return variables have to match
625+
// unless we have a public function overriding an external one.
626+
if (
627+
_overriding.isFunction() &&
628+
!returnTypesDifferAlready &&
629+
_super.visibility() != Visibility::External &&
630+
_overriding.functionKind() != Token::Fallback
631+
)
632+
{
633+
if (!_overriding.originalFunctionType()->hasEqualParameterTypes(*_super.originalFunctionType()))
634+
overrideError(
635+
_overriding,
636+
_super,
637+
7723_error,
638+
"Data locations of parameters have to be the same when overriding non-external functions, but they differ.",
639+
"Overridden " + _overriding.astNodeName() + " is here:"
640+
);
641+
if (!_overriding.originalFunctionType()->hasEqualReturnTypes(*_super.originalFunctionType()))
642+
overrideError(
643+
_overriding,
644+
_super,
645+
1443_error,
646+
"Data locations of return variables have to be the same when overriding non-external functions, but they differ.",
647+
"Overridden " + _overriding.astNodeName() + " is here:"
648+
);
607649
}
608650

609651
// Stricter mutability is always okay except when super is Payable

‎libsolidity/analysis/OverrideChecker.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,10 @@ class OverrideProxy
8484
/// @returns receive / fallback / function (only the latter for modifiers and variables);
8585
langutil::Token functionKind() const;
8686

87-
FunctionType const* functionType() const;
87+
/// @returns the externally callable function type
88+
FunctionType const* externalFunctionType() const;
89+
/// @returns the (unmodified) function type
90+
FunctionType const* originalFunctionType() const;
8891
ModifierType const* modifierType() const;
8992

9093
Declaration const* declaration() const;
@@ -101,6 +104,7 @@ class OverrideProxy
101104

102105
/**
103106
* Struct to help comparing override items about whether they override each other.
107+
* Compares functions based on their "externally callable" type.
104108
* Does not produce a total order.
105109
*/
106110
struct OverrideComparator

‎libsolidity/ast/AST.cpp

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,9 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual(
480480
solAssert(isOrdinary(), "");
481481
solAssert(!libraryFunction(), "");
482482

483-
FunctionType const* functionType = TypeProvider::function(*this)->asExternallyCallableFunction(false);
483+
// We actually do not want the externally callable function here.
484+
// This is just to add an assertion since the comparison used to be less strict.
485+
FunctionType const* externalFunctionType = TypeProvider::function(*this)->asExternallyCallableFunction(false);
484486

485487
bool foundSearchStart = (_searchStart == nullptr);
486488
for (ContractDefinition const* c: _mostDerivedContract.annotation().linearizedBaseContracts)
@@ -495,9 +497,12 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual(
495497
// With super lookup analysis guarantees that there is an implemented function in the chain.
496498
// With virtual lookup there are valid cases where returning an unimplemented one is fine.
497499
(function->isImplemented() || _searchStart == nullptr) &&
498-
FunctionType(*function).asExternallyCallableFunction(false)->hasEqualParameterTypes(*functionType)
500+
FunctionType(*function).asExternallyCallableFunction(false)->hasEqualParameterTypes(*externalFunctionType)
499501
)
502+
{
503+
solAssert(FunctionType(*function).hasEqualParameterTypes(*TypeProvider::function(*this)));
500504
return *function;
505+
}
501506
}
502507

503508
solAssert(false, "Virtual function " + name() + " not found.");

‎test/externalTests/bleeps.sh

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,14 @@ function bleeps_test
8686
npm install npm-run-all
8787
npm install
8888

89+
# TODO: Bleeps depends on OpenZeppelin 4.3.2, which is affected by
90+
# https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3293.
91+
# Forcing OZ >= 4.6.0 fixes this but it also causes a lot of unrelated compilation errors.
92+
# Remove this when Bleeps gets updated to support newer OpenZeppelin.
93+
perl -i -0pe \
94+
"s/(function hashProposal\(\n address\[\] )calldata( targets,\n uint256\[\] )calldata( values,\n bytes\[\] )calldata( calldatas,)/\1memory\2memory\3memory\4/g" \
95+
node_modules/@openzeppelin/contracts/governance/IGovernor.sol
96+
8997
replace_version_pragmas
9098

9199
for preset in $SELECTED_PRESETS; do

0 commit comments

Comments
 (0)