Platform: Code4rena
Start Date: 15/07/2021
Pot Size: $80,000 USDC
Total HM: 28
Participants: 18
Period: 7 days
Judge: ghoulsol
Total Solo HM: 18
Id: 20
League: ETH
Rank: 14/18
Findings: 2
Award: $408.77
🌟 Selected for report: 0
🚀 Solo Findings: 0
heiho1
Pool.removeForMember(address), Pool.swapTo(address,address) and Pool.burnSynth(address,address) on lines 198, 199, 224, 250, and 253 ignore the boolean return on transfers. This is a brittle implementation because it relies on the boolean return value being hard-coded to true. Potentially a token may return a false instead of reverting and this opens the possibility of unexpected outcomes for removals, swaps and burns.
Note also that Pool.swapTo(address,address) emits a potentially erroneous Swapped() event on line 223 due to the above unchecked transfer.
Slither
There is no particular disadvantage to a require(success, "!transfer") check.
Events should not be emitted prior to confirmation of successful transfer or the logs may become polluted.
#0 - SamusElderg
2021-07-22T02:53:36Z
Duplicate of #8
heiho1
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.
https://github.com/ethereum/go-ethereum/issues/19930
Slither
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.
#0 - SamusElderg
2021-07-26T02:47:15Z
Different to #132
#1 - SamusElderg
2021-07-26T03:57:04Z
Based on this and other similar to other issues brought up throughout this review; we will consider adding OpenZeppelin's nonReentrant to payables
#2 - SamusElderg
2021-07-29T03:09:53Z
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
#3 - ghoul-sol
2021-08-08T18:37:20Z
I don't see description how reentrancy can damage the protocol so I'm making this a duplicate of #46
177.3903 USDC - $177.39
heiho1
Pool.constructor(address,address) lines 44 and 45 accept possibly zero address for base/token
Slither
If base/token is to be treated as zero address, it should be positively checked. If not then it should be negatively checked.
#0 - verifyfirst
2021-07-26T01:27:21Z
Checks are done in the poolFactory
#1 - verifyfirst
2021-07-26T01:49:53Z
This is a duplicate of #147
#2 - ghoul-sol
2021-08-08T19:57:20Z
Duplicate of #147 so low risk