Spartan Protocol contest - k's results

Handle

k

Vulnerability details

Impact

This vulnerability could allow a user to misrepresent the amount of tokens transferred to the protocol, either intentionally or by accident. Under certain conditions this would allow a user to bond tokens in the DAO without actually transferring any tokens to the DAO. As a result, a malicious would be able to drain the BondVault by bonding the maximum amount of tokens possible without providing any such tokens.

While all calls to BEP20 token transfer and transferFrom functions should validate the returned value to ensure that the transfer succeeded, the Dao.bond() function was found to be the most likely instance in scope to be exploitable (https://github.com/code-423n4/2021-07-spartan/blob/e2555aab44d9760fdd640df9095b7235b70f035e/contracts/Dao.sol#L266). This was because the asset parameter was likely an external token, which may not conform to the expected revert logic, e.g. when a user tries to transfer more tokens than they have the function could return false instead of reverting.

A mitigating factor for the bug, which lowered it from a high to medium risk, was that an access control list was maintained, which only allowed assets in the isAsset mapping to be bonded. SamusElderg confirmed on Discord that the initial list would include BNB, BUSD, USDT, and BTCB, which meant that the initial list would not be susceptible to the bug described, however, the addition of more assets at a later stage could weaponize the attack.

A number of return statements were already properly validated, which lends credibility to the fact that this was an oversight by the team. Barring the Dao.bond() function, each of the other instances where validation was not performed had mitigating factors to the extent that the bug could likely not be exploited under any condition, such as explicitly using tokens that revert on failed transfers (e.g. synth and pool tokens) or using separate logic to track the actual amount of tokens transferred (e.g. Synth.sol#L204-205 where an "actual" value is established).

Proof of Concept

N/A

Tools Used

N/A

Recommended Mitigation Steps

All calls to BEP20 transfer or transferFrom functions should at least be wrapped in a require statement to properly validate the returned value. As a further step, the OpenZeppelin SafeERC library can be used for external tokens, as some tokens may not necessarily properly conform to the BEP20 standard and return any data at all (further reading: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca).