search

code-423n4 / 2021-09-defiprotocol-findings

1 stars 0 forks source link

Resetting partial struct fields is risky #189

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

There are multiple places where individual fields of struct are reset to default values. This is risky because it may lead to use of stale values left over in other struct fields in other/later logic/flows.

A cleaner approach is to delete the entire struct to make sure all its fields are set to default values and minimize the risk of any stale values being used in any flows. This also clearly indicates the intent of the developer.

Proof of Concept

https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Basket.sol#L141

https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Basket.sol#L159

https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Basket.sol#L199

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use delete on entire structs (recognizing that any mapping fields are not cleared as per Solidity semantics).

GalloDaSballo commented 2 years ago

Agree with finding, not sure if informational (non-critical) as there's no poc here

Will leave as low severity for now as I believe that this can cause issues, but we don't have a specific example of how