search

code-423n4 / 2021-11-nested-findings

1 stars 1 forks source link

NestedBuybacker sends NST to NestedReserve with no proper way to retrieve it. #33

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

TomFrench

Vulnerability details

Impact

Co-mingling of DAO and user funds for no reason. Requirement to deploy a specialised factory to retrieve funds and store them somewhere more suitable.

Proof of Concept

When triggered the NestedBuybacker sends NST to the NestedReserve

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedBuybacker.sol#L37-L38

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedBuybacker.sol#L106-L113

This places this NST in the same contract as user owned NST with no proper bookkeeping (the amount of NST owned by the DAO needs to be calculated offchain). There's also no functions to retrieve this NST without impersonating a factory with a new contract specially deployed for the task.

https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedReserve.sol

Recommended Mitigation Steps

Have a separate treasury contract to hold this NST which has been bought which allows safe withdrawal of this NST without the potential for accidentally stealing user funds.

adrien-supizet commented 3 years ago

The nstReserve mentioned here https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedBuybacker.sol#L106-L113 is an EOA and not the NestedReserve contract used to store funds.

The wording can be confusing and can be adapted, but this is not an issue.

alcueca commented 2 years ago

Downgraded to code clarity issue.