search

code-423n4 / 2021-07-spartan-findings

0 stars 0 forks source link

Pool.swapTo(address token, address member) potentially locks funds #202

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

heiho1

Vulnerability details

Impact

Pool.swapTo(address token, address member) is payable and potentially reentrant. This potentially allows users to erroneously lock funds in the contract. There is no clear mechanism by which such funds could be recovered.

Proof of Concept

https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Pool.sol#L211

https://github.com/ethereum/go-ethereum/issues/19930

Tools Used

Slither

Recommended Mitigation Steps

The function is public and so should be marked nonReentrant, i.e. using OpenZeppelin:

https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard

It is also advisable to consider a selfdestruct() implementation so that locked funds can at least be retrieved and possibly redistributed to minimize unintentional loss.

https://solidity-by-example.org/hacks/self-destruct/

SamusElderg commented 3 years ago

Different to #132

SamusElderg commented 3 years ago

Based on this and other similar to other issues brought up throughout this review; we will consider adding OpenZeppelin's nonReentrant to payables

SamusElderg commented 3 years ago

Just realised this is a function that does not handle BNB so 'it does not need to be payable' would be the issue which would be low risk IMO; mitigation is to remove 'payable' no BNB would have been at risk if the user was using the functions as intended

Duplicate of #46

ghoul-sol commented 3 years ago

I don't see description how reentrancy can damage the protocol so I'm making this a duplicate of #46