Possiblity of storage collision

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cCash/CCash.sol#L16

Vulnerability details

Impact

There is a possibility of storage collision, when you upgrade the implementation contract in the https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cCash/CCash.sol#L16. This could happen because the storage in the smart contract is stored the storage value from the left to the right, and if you add a new var to the storage contract it will push the storage value in the second inheritance to the next slot.

Proof of Concept

This is a simple POC that could demonstrate it:

pragma solidity =0.8.7;

contract contract3 {
    uint256 internal _storage3 = 30;
}

contract contract2 {
     uint256 internal _storage2 = 20;
     //uint256 internal storageAddition = 200000;
}

abstract contract abstractContract is contract2, contract3 {

}

contract contract1 is abstractContract{

    uint256 internal _storage1 = 10;

    function checkSlot(uint256 _slot)external view returns(uint256 result){
        assembly {
            result := sload(_slot)
        }
    }
}

if you try this code demonstration, you can see that the storage value can be mapped as.

Slot 0 = 20
Slot 1 = 30
Slot 2 = 10
Slot 3 = 0

And if you upgrade the contract by uncomment the storageAddition, the storage mapped would be

Slot 0 = 20
Slot 1 = 200000
Slot 2 = 30
Slot 3 = 10

as you can see the value 10 is being push to the slot 3.

And as you can see in this snippet of code contract CCash is CTokenCash, CErc20Interface the CCash contract inherit the CtokenCash which inherit CTokenInterface and then inherit CTokenStorage which mapped the storage layout, and then the CErc20Interface inherit CErc20Storage which will stored the underlying var. from this we can mapped the storage layout as this.

Slot 0 = _notEntered var::CTokenStorage 
Slot 1 = name var::CTokenStorage 
...
...
...
Slot n = accountBorrows var::CTokenStorage 
Slot n+1 = underlying var::CErc20Storage 

If you add a new variable in the CTokenStorage, whether you are trying to add some feature or what not, the underlying var will be push to the slot n+2, and now everytime the contract call underlying it would be address(0) the mapped storage layout would be

Slot 0 = _notEntered var::CTokenStorage 
Slot 1 = name var::CTokenStorage 
...
...
...
Slot n = accountBorrows var::CTokenStorage
Slot n+1 = newStorage var::CTokenStorage
Slot n+2 = underlying var::CErc20Storage 

Since you are using a proxy for this smart contract, you have to be mindful of the storage layout of the proxy, so it's not pointing to the wrong slot

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope

[c4-sponsor]

tom2o17 marked the issue as sponsor disputed

[tom2o17]

Compound upgradable contracts comply with EIP-897 standard for upgradable contracts. As such Implementation contracts are aware of the storage layout of the proxy contract.

Any upgrades made to the token will be in the following pattern here.

Which should not break the storage layout set forth by the initial implementation contract.

cc @ypatil12 @cameronclifton