istambul fix #91
istambul fix #91
Conversation
There was a problem hiding this comment.
I don't think there's any risk from reentrancy here, as we delete a struct which is required at the start of each function. However, I would suggest adding a reentrancy guard, eg the modifier nonReentrant which is in the AragonApp contract, to each function that does a call.value().
| @@ -31,6 +31,7 @@ contract TokenRequest is AragonApp { | |||
| string private constant ERROR_ADDRESS_NOT_CONTRACT = "TOKEN_REQUEST_ADDRESS_NOT_CONTRACT"; | |||
| string private constant ERROR_TOKEN_REQUEST_NOT_OWNER = "TOKEN_REQUEST_NOT_OWNER"; | |||
| string private constant ERROR_ETH_VALUE_MISMATCH = "TOKEN_REQUEST_ETH_VALUE_MISMATCH"; | |||
| string private constant ERROR_ETH_TRANSFER_FAILED = "TOKEN_REQUEST_ETH_TRANSFER_FAILED"; | |||
There was a problem hiding this comment.
Can we add a test for this new error message?
|
@willjgriff i did as you requested a test for when the ETH transfer fails, but for when we are doing the refund i couldn't make it work because i am setting the msg.sender as mockErc20.address in order to make it fail but i am getting this other error |
This is because of bingen's comment
Because of those problems with EIP-1884, you may consider not usingtransfer. See: <https://github.com/aragon/aragonOS/pull/551> <https://github.com/aragon/aragon-apps/issues/1017> <https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/>